Opened 7 weeks ago

Closed 5 weeks ago

#31822 closed defect (fixed)

Security slider is not really visible on mobile anymore (just some dots in the menu)

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-9.0, TorBrowserTeam201910R
Cc: sysrqb Actual Points: 0.1
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

With the switch to esr68 the security slider on mobile is not really a slider anymore: there are only dots visible for the respective levels.

Child Tickets

Change History (20)

comment:1 Changed 7 weeks ago by gk

Note we have #31776. However, we might want to fix that bug for 9.0 already if we have the time to do so.

comment:2 Changed 7 weeks ago by sysrqb

Interesting. This is a rendering bug in fennec. I created a test page and it renders correctly on desktop but not Android (neither Fennec or Tor Browser).

http://sbe5fi5cka5l3fqe.onion/~sysrqb/31822/test.html
https://people.torproject.org/~sysrqb/31822/test.html

comment:3 Changed 7 weeks ago by sysrqb

This broke in the 2018-11-16 nightly build.

comment:4 Changed 7 weeks ago by sysrqb

Okay, probably either Bug 1317870 or Bug 1481593. I'm going to open a geckoview bug for this.

comment:6 Changed 7 weeks ago by sysrqb

Keywords: TorBrowserTeam201909R added
Status: newneeds_review

I have a torbutton patch for this (in my github repo). Branch bug31822.

It looks like there may be a patch incoming from Mozilla, as well. They're reverting the (piece of the) change that introduced this.

comment:7 Changed 7 weeks ago by sysrqb

For some reasoning behind the torbutton branch, from the bugzilla ticket:

Bug 1481593 most likely broke this by removing the background. That change is
intentional though, because we want -webkit-appearance: none to not render the
range track. It's basically impossible to support that behavior though without
having a native theme.

Branch bug31822 simply re-adds the background and track color on the slider.

comment:8 Changed 7 weeks ago by gk

Hrm. Why don't we go with the Mozilla patch? The advantages I see here are two-fold: 1) we'd get the official fix which has tests making sure it is correct. 2) We won't clutter our Torbutton code with workarounds but would rather have the backport in tor-browser. That's a plus in that we have a fix less to carry around automatically when rebasing to a new Firefox version that actually contains the fix (otherwise we'd need to write a separate patch ourselves to remove the workaround in Torbutton).

comment:9 Changed 7 weeks ago by sysrqb

The Mozilla patch wasn't posted until yesterday, and I finished testing my patch around the same time as that. We should take Mozilla's patch, but it's not reviewed yet.

comment:10 Changed 7 weeks ago by Thorin

FYI: You guys might find this useful: http://stephenhorlander.com/form-controls.html

I'm using a modified version of that, and another for all types and states of all elements in some new entropy tests (to detect app language leaks). Will keep you posted.

comment:11 Changed 7 weeks ago by cypherpunks

And where are the unstyled checkboxes and radio buttons? TBB 8.5.5.

comment:12 Changed 7 weeks ago by Thorin

IDK: I'll check with Stephen (when I get to it) - here's the backstory... https://github.com/ghacksuserjs/TorZillaPrint/issues/33#issuecomment-491425131

comment:13 in reply to:  9 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

Replying to sysrqb:

The Mozilla patch wasn't posted until yesterday, and I finished testing my patch around the same time as that. We should take Mozilla's patch, but it's not reviewed yet.

Let's wait for that then.

comment:14 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201910 added

comment:15 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201909 removed

comment:16 Changed 6 weeks ago by sysrqb

Points: 0.5
Status: needs_revisionnew

Hopefully the backport won't take much time.

comment:17 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

I saw the Mozilla bug got fixed and cherry-picked https://hg.mozilla.org/mozilla-central/rev/34a3e14b45b5 back to our esr68 branch. Please have a look at bug_31822 (https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_31822&id=f084920c8e571de1f6d9e50a5a6c00c13b65ed4c). (I've not built/tested the backport)

comment:18 Changed 5 weeks ago by sysrqb

This patch looks good. I was waiting until they uplifted it, but it seems this is taking longer than expected and there shouldn't be any merge conflicts.

I'll test this and confirm it works, and then we can merge it.

comment:19 in reply to:  18 ; Changed 5 weeks ago by sysrqb

Status: needs_reviewmerge_ready

Replying to sysrqb:

This patch looks good. I was waiting until they uplifted it, but it seems this is taking longer than expected and there shouldn't be any merge conflicts.

Okay, it landed on mozilla-esr68, too. There isn't a significant difference between the patches, we can take either of them.

I'll test this and confirm it works, and then we can merge it.

And it works for me!

comment:20 in reply to:  19 Changed 5 weeks ago by gk

Actual Points: 0.1
Resolution: fixed
Status: merge_readyclosed

Replying to sysrqb:

Replying to sysrqb:

This patch looks good. I was waiting until they uplifted it, but it seems this is taking longer than expected and there shouldn't be any merge conflicts.

Okay, it landed on mozilla-esr68, too. There isn't a significant difference between the patches, we can take either of them.

I'll test this and confirm it works, and then we can merge it.

And it works for me!

Nice that Mozilla moved so fast here. I think using the patch that landed on esr68 for our next alpha is preferable as we then test what actually gets shipped in Tor Browser 9 once we rebase to Mozilla's ESR branch. Thus, I cherry-picked the fix that landed on top of tor-browser-68.1.0esr-9.0-2 (commit 339576fa544c87058d34337547fea2d5c0c926da).

I set my points, sysrqb, please add yours.

Note: See TracTickets for help on using tickets.