Closed Bug 1274272 Opened 9 years ago Closed 9 years ago

[RTL] Fix new private window in RTL locales

Categories

(Firefox :: Private Browsing, defect)

Product:
Firefox
Component:
Private Browsing
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: magicp.jp, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(3 files)

rtl-new-private-window.png
45.52 KB, image/png
Details
MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer
58 bytes, text/x-review-board-request
mikedeboer
: review+
Sylvestre
: approval-mozilla-aurora+
Details
followup: use float: left/right to ensure this keeps working when inline-start is not available,
2.12 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160519030232 Steps to reproduce: 1. Start Nightly in RTL locales (e.g. arabic) 2. Open new private window Actual results: No css support in RTL locales. Expected results: Should support with RTL locales.
Blocks: 1259340
Has STR: --- → yes
status-firefox48: --- → affected
status-firefox49: --- → affected
Component: Untriaged → Private Browsing
OS: Unspecified → All
Hardware: Unspecified → All
:ntim, I think you had a partial patch for this already. Any chance you want to finish it off?
Flags: needinfo?(ntim.bugs)
(In reply to :Gijs Kruitbosch from comment #1) > :ntim, I think you had a partial patch for this already. Yes, the latest patch from bug 1267047 implements this. I believe we can simply extract the RTL parts of that patch into its own patch to fix this bug. > Any chance you want to finish it off? Sorry, I'm busy this month with exams. This bug does seem like a good candidate for a good first/mentored bug though.
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen :ntim (busy, email me if needed) from comment #2) > (In reply to :Gijs Kruitbosch from comment #1) > > :ntim, I think you had a partial patch for this already. > Yes, the latest patch from bug 1267047 implements this. I believe we can > simply extract the RTL parts of that patch into its own patch to fix this > bug. > > > Any chance you want to finish it off? > Sorry, I'm busy this month with exams. This bug does seem like a good > candidate for a good first/mentored bug though. There's time-pressure on it, so that doesn't seem great - 48 will move to beta in 2 weeks. I'll have a look myself, I guess.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Keywords: regression
(NB: you can easily test this with an English-language Firefox by flipping the dir attribute on the <body> to be "rtl".)
https://reviewboard.mozilla.org/r/54044/#review50776 Drive by comment, thanks for picking this up! ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:176 (Diff revision 1) > } > > +.toggle + .toggle-btn:dir(rtl)::after { > + right: 0; > + left: auto; > + transition: right .2s ease; Maybe it would be better to just override `transition-property`, to avoid redefining the rest twice?
Comment on attachment 8754558 [details] MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54044/diff/1-2/
Comment on attachment 8754558 [details] MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer https://reviewboard.mozilla.org/r/54044/#review50918 ::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:174 (Diff revision 2) > background-color: transparent; > background-image: url("chrome://browser/skin/privatebrowsing/check.svg"); > } > > +.toggle + .toggle-btn:dir(rtl)::after { > + right: 0; nit: can you sort these alphabetically too?
Attachment #8754558 - Flags: review?(mdeboer) → review+
Comment on attachment 8754558 [details] MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54044/diff/2-3/
Attachment #8754558 - Attachment description: MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r?mikedeboer → MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer
Comment on attachment 8754558 [details] MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer Approval Request Comment [Feature/regressing bug #]: bug 1259340 [User impact if declined]: the tracking protection toggle is not usable for RTL users, and the page looks broken [Describe test coverage new/current, TreeHerder]: no, style changes only [Risks and why]: very low, style changes only applicable to rtl. Tested by several people, would be hard to make RTL any worse than it is [String/UUID change made/needed]: none
Attachment #8754558 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/392304ae5264
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8754558 [details] MozReview Request: Bug 1274272 - fix RTL issues with about:privatebrowsing, r=mikedeboer Improve RTL, taking it.
Attachment #8754558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Aurora doesn't seem to like some of the css changes here: https://treeherder.mozilla.org/logviewer.html#?job_id=2664196&repo=mozilla-aurora MDN says 'float:inline-start' has been allowed on nightly and aurora since fx45, so I'm not sure why that's complaining. :dir seems to be nightly-exclusive, according to MDN. Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/349688fd0b53
status-firefox48: fixed → affected
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Wes Kocher (:KWierso) from comment #15) > Aurora doesn't seem to like some of the css changes here: > https://treeherder.mozilla.org/logviewer.html#?job_id=2664196&repo=mozilla- > aurora > MDN says 'float:inline-start' has been allowed on nightly and aurora since > fx45, so I'm not sure why that's complaining. > :dir seems to be nightly-exclusive, according to MDN. > > Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/349688fd0b53 D'oh. This also means we might need to update m-c code because it'll break when we merge to aurora. I'll do that as well... https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2426-2429 indicates float: inline-start is gated to NIGHTLY_BUILD, which I'm fairly sure isn't defined on aurora, so I suspect MDN is just wrong. :jfkthame, am I missing something? Is there a reason to keep that gated? :dir on Nightly is shipping per https://groups.google.com/d/topic/mozilla.dev.platform/F0_UbXAfB_4/discussion so I don't think that needs changing on m-c, but I'll update the aurora patch to use :-moz-dir instead.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jfkthame)
(In reply to :Gijs Kruitbosch from comment #16) > https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all. > js#2426-2429 > > indicates float: inline-start is gated to NIGHTLY_BUILD, which I'm fairly > sure isn't defined on aurora, so I suspect MDN is just wrong. :jfkthame, am > I missing something? Is there a reason to keep that gated? We didn't ship it to release channels because of an unresolved spec issue (see bug 1253919). I think the intention was to ship it on Nightly and Dev Edition, though (as MDN suggests), which means the condition in all.js is wrong -- if you'd like to post a patch to change NIGHTLY_BUILD to !RELEASE_BUILD there, I'll happily r+ it. (I'd really like to get bug 1253919 resolved so we could ship this to all channels, but have not had much success trying to get a clear decision from the CSSWG...)
Flags: needinfo?(jfkthame)
MozReview-Commit-ID: 7VTz8mwyUN7
Attachment #8756839 - Flags: review?(mdeboer)
Attachment #8756839 - Flags: review?(mdeboer) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: