Closed Bug 1274272 Opened 8 years ago Closed 8 years ago

[RTL] Fix new private window in RTL locales

Categories

(Firefox :: Private Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: magicp.jp, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(3 files)

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
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: 8 years ago
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
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.