Closed
Bug 1274272
Opened 8 years ago
Closed 8 years ago
[RTL] Fix new private window in RTL locales
Categories
(Firefox :: Private Browsing, defect)
Firefox
Private Browsing
Tracking
()
RESOLVED
FIXED
Firefox 49
People
(Reporter: magicp.jp, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(3 files)
45.52 KB,
image/png
|
Details | |
58 bytes,
text/x-review-board-request
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
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
Assignee | ||
Comment 1•8 years ago
|
||
:ntim, I think you had a partial patch for this already. Any chance you want to finish it off?
Flags: needinfo?(ntim.bugs)
Comment 2•8 years ago
|
||
(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)
Assignee | ||
Comment 3•8 years ago
|
||
(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 | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54044/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54044/
Attachment #8754558 -
Flags: review?(mdeboer)
Assignee | ||
Comment 5•8 years ago
|
||
(NB: you can easily test this with an English-language Firefox by flipping the dir attribute on the <body> to be "rtl".)
Comment 6•8 years ago
|
||
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?
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 11•8 years ago
|
||
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?
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/392304ae5264
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 13•8 years ago
|
||
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
Flags: needinfo?(gijskruitbosch+bugs)
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/a10c6f6843af
Assignee | ||
Comment 16•8 years ago
|
||
(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)
Assignee | ||
Comment 17•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/e6b03d718152
Comment 18•8 years ago
|
||
(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)
Assignee | ||
Comment 19•8 years ago
|
||
MozReview-Commit-ID: 7VTz8mwyUN7
Attachment #8756839 -
Flags: review?(mdeboer)
Updated•8 years ago
|
Attachment #8756839 -
Flags: review?(mdeboer) → review+
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a75edd0bfb2
You need to log in
before you can comment on or make changes to this bug.
Description
•