Closed Bug 1194346 Opened 6 years ago Closed 6 years ago

[RTL] Language labels are shifted in preferences

Categories

(Firefox :: Preferences, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox40 + wontfix
firefox41 + wontfix
firefox42 - affected
firefox43 - affected
firefox45 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- wontfix

People

(Reporter: safa1996alfulaij, Unassigned, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: regression, rtl)

Attachments

(9 files, 2 obsolete files)

28.26 KB, image/png
Details
571 bytes, application/vnd.mozilla.xul+xml
Details
553.43 KB, video/ogg
Details
1.45 MB, video/ogg
Details
1.50 KB, patch
jfkthame
: feedback-
Details | Diff | Splinter Review
90.00 KB, image/png
Details
93.38 KB, image/png
Details
2.39 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
53.73 KB, image/png
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.21 (KHTML, like Gecko) QupZilla/1.8.6 Safari/537.21

Steps to reproduce:

In RTL locale:
Open the preferences tab.
Go to Content tab.
Click "Select..." button in the languages section.
Add some languages.


Actual results:

The labels are shifted to right and can't be seen.


Expected results:

The labels should stay in box without being shifted.
The labels can be seen if the window got maximized (the languages window).
[Tracking Requested - why for this release]: Broken layout

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=78282059eb4a&tochange=1b86fdc1e49f

Suspect:
4c0ccf99cb71	Daniel Holbert — Bug 1131371: Only update overflow region (& trigger DLBI) when CSS "outline" changes, instead of triggering a reflow. r=heycam
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dholbert)
Keywords: regression, rtl
Version: 40 Branch → 38 Branch
Thanks -- I'll take a look tomorrow.
I can reproduce this bug, using the Hebrew version of Firefox Dev Edition downloaded from here:
 https://www.mozilla.org/en-US/firefox/developer/all/

When I perform the STR in that build, the "Languages" section in preferences already has 4 entries (which is sufficient to see a scrollbar), and they're already shifted out of view to the right when the popup appears. (And I can make them adjust their positions & appear simply by scrolling the widget up & down.)
Attached file testcase 1
So, I think this is really an old bug with rtl XUL listboxes, which we perhaps were accidentally hacking around in this particular context (in about:preferences) for a while, but bug 1131371 made that hackaround no longer work.

In particular:
 - THE OLD BUG: if you reduce the size of an RTL xul listbox, then its contents will be cut off (i.e. they aren't repositioned), until you scroll the listbox. See attached testcase which demonstrates this. (Note: you have to save it locally and add pref "dom.allow_XUL_XBL_for_file" for it to load, due to security restrictions on XUL.)  This testcase exhibits this issue in builds as old as Firefox 15, and probably much older builds as well.  I'm guessing this preferences pane is triggering a version of this bug. (albeit without any visible resizing)

 - THE ACCIDENTAL WORKAROUND: Until my patch in bug 1131371 landed, adding an 'outline' would trigger a reflow. When you pop up the dialog in this bug's STR, one of the list items is highlighed & outlined -- so presumably that outline forced a reflow, which updated the positions of everything & worked around the old bug. But outlines no longer cause a reflow, so that doesn't work anymore.

So, we probably need to (1) make sure there's a bug filed for this old listbox issue, and (2) find a new workaround for this preferences pane to use.
Here's a screencast of the bug, with testcase 1, in standard Nightly. (no need for any particular localization)
Flags: needinfo?(dholbert)
Note: If I select one of the list items, then the bug *seems* to go away in older versions of Firefox, which I think is simply because the selected item has an "outline", which gets un-applied while the window is being resized, and re-applied when I stop resizing. (And that triggers a reflow, up until bug 1131371.)
When enabled APZ, the behavior is different. 
Some of characters are hidden even if after scroll the list.
And a fake scrollbar is drawn at the right side of list.
(In reply to Alice0775 White from comment #7)
> When enabled APZ, the behavior is different.
> Some of characters are hidden even if after scroll the list.

Interesting. That makes sense, I think. (With APZ, we're just moving around a texture without redoing any layout inside of it. So the broken positions stay broken.)

> And a fake scrollbar is drawn at the right side of list.

Hmm, that seems like another bug here.
Depends on: 1194844
I filed bug 1194844 on the underlying rtl-listbox issue (for contents being incorrectly positioned after a resize).

Alice0775, would you mind filing a new bug on the APZ fake-scrollbar-space issue that you brought up?

And we can leave this bug here on fixing the preference pane (perhaps with a hack of some sort to force reflow & clear out the mispositioning, as 'outline' used to do but no longer does).
Component: Layout: Text → Preferences
Product: Core → Firefox
(In reply to Daniel Holbert [:dholbert] from comment #9)

> 
> Alice0775, would you mind filing a new bug on the APZ fake-scrollbar-space
> issue that you brought up?
> 

Filed Bug 1194851
Tracking for 43 since this is a regression (from Firefox 38, as described in comment 4)

If we want to add another workaround for the preferences labels let's also think about how far to uplift it. Would it be better to tackle the underlying issue?
Too late for 40 and this bug won't be fixed in ESR (we only take security fixes and top critical issues).
While this is a valid bug, resizing the window refreshes the layout. I think we can live with that workaround in FF41 timeframe. Setting 41 status as wontfix.

I hope we find a fix in time for FF42 release.
We published (at least) two releases with this bug, no duplicate, not a big deal, untracking?
Duplicate of this bug: 1220440
Three ideas for workarounds:

(1) Easy: just force this particular listbox to be LTR. It might be a bit disconcerting, but it's arguably better than having missing/clipped content.

(2) Not-sure-how-hard: Use an HTML listbox instead of a XUL one here. HTML listboxes don't seem to be susceptible to this problem. Sample: <select size="2" style="width: 100%" dir="rtl"><option>Hello</option><option>Hello</option><option>Hello</option>
(3) Adjust the preferences-pane code to force a reflow somehow (not sure what a surefire way to do that is, in XUL) at times when this bug might be triggered (any time this widget might have been resized?)

and of course one more option is:
(4) Fix the underlying XUL bug, bug 1194844
Unfortunately, I don't have cycles to dig in too deeply, but I feel like we should do *something* here.  I'm tempted to do (1) -- patch attached.

This is clearly suboptimal, but it means we'll at least show this listbox's contents instead of clipping them, so it's pretty-clearly better than what we're doing right now.  (Also: this piece of UI is pretty buried, so even if the LTR-ness here seems a little weird to RTL-language users, it'll still be a pretty small fraction of users that even ever see this piece of UI.)

Jonathan, does this seem OK to you?  (I should probably get actual review from a Firefox frontend reviewer, but tagging you for feedback first.)
Attachment #8683243 - Flags: feedback?(jfkthame)
Here's a screenshot showing what this looks like without the patch (with rtl UI forced by setting intl.uidirection.en to rtl, to emulate having an RTL language-pack).
...and here's a screenshot with the patch (again, with intl.uidirection.en set to rtl)
FWIW, I would personally prefer to use an HTML listbox, because eventually we should migrate these XUL pages to HTML anyway. Might as well start where we can.
Comment on attachment 8683243 [details] [diff] [review]
hackaround patch v1: force this listbox to be LTR

Review of attachment 8683243 [details] [diff] [review]:
-----------------------------------------------------------------

I'd be OK with the idea of a hacky workaround here, in the absence of a real fix for the underlying bug (and I don't have cycles to go looking for that at the moment, either), but I don't much like this particular workaround.

I wonder if we could do something (e.g. in gLanguagesDialog.init()) that would trigger the extra reflow we need to make things better?

(Or if Gijs or someone is up for replacing the XUL listbox with HTML, so much the better.)
Attachment #8683243 - Flags: feedback?(jfkthame) → feedback-
:gijs, can you take this bug, or know someone else who can work on it?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jonathan Kew (:jfkthame) from comment #24)
> :gijs, can you take this bug, or know someone else who can work on it?

I don't know offhand how much work is involved, have a number of other things on my plate that are more important (for some meaning of the word) and I'm a bit too spread out as it is. However, I think that the conversion of this to HTML might interest Archaeopteryx, who's done a bunch of work on the prefs in the past. ("No, it doesn't" is a fair answer, then please just needinfo me back again, aryx :-) )
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(aryx.bugmail)
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> I'd be OK with the idea of a hacky workaround here, in the absence of a real
> fix for the underlying bug (and I don't have cycles to go looking for that
> at the moment, either), but I don't much like this particular workaround.

Fair enough.

> I wonder if we could do something (e.g. in gLanguagesDialog.init()) that
> would trigger the extra reflow we need to make things better?

Maybe! That'd be a version of workaround (3) suggested above. Notes on that:
  - Since adjusting the width doesn't trigger a reflow (like it would in HTML), I'm not sure what other tweaks might work here to get us to reflow *inside* the listbox here.  (Maybe a font-size tweak, followed by a layout flush (w/ e.g. someElem.offsetTop), and then undo the font-size tweak?)
  - Whatever tweak we do, we kinda need it to happen whenever the window is resized, for robustness... (since this listbox grows/shrinks with window-size)  So we might need to set up some sort of listener to catch that.  But, maybe it'd be good enough to just get the rendering right up-front & throw up our hands if the window is resized...

But I agree that conversion to HTML seems like a more ideal fix.
For the record, I just posted a new "testcase 2" on bug 1194844 which confirms that HTML listboxes (<select size="...">) do not have this issue.

However, they do hit a less-severe thing mentioned on this bug -- there's a strip of un-painted space on the right. Alice0775 noticed that earlier here & spun off Bug 1194851 to cover that, and happily it looks like that bug might be getting some attention (tn assigned it to himself recently, at least).
Ah, no need to worry much about that strip of unpainted space mentioned in comment 27 -- that only happens if APZ (layers.async-pan-zoom.enabled) is turned on, and APZ is only turned on by default on Nightly for now (via an #ifdef).  And bug 1194851 (the bug about the strip of unpainted space) is marked as blocking us from letting APZ ride the trains.

So, if we fix this bug here by switching to a HTML listbox, we don't need to worry about non-Nightly users tripping over that unpainted-strip issue.
(In reply to Daniel Holbert [:dholbert] from comment #26)
>   - Since adjusting the width doesn't trigger a reflow (like it would in
> HTML), I'm not sure what other tweaks might work here to get us to reflow
> *inside* the listbox here.  (Maybe a font-size tweak, followed by a layout
> flush (w/ e.g. someElem.offsetTop), and then undo the font-size tweak?)

Yeah, something along those lines seems like it might work (though untested).

>   - Whatever tweak we do, we kinda need it to happen whenever the window is
> resized, for robustness... (since this listbox grows/shrinks with
> window-size)  So we might need to set up some sort of listener to catch
> that.

Good point.

>  But, maybe it'd be good enough to just get the rendering right
> up-front & throw up our hands if the window is resized...

I could live with that temporarily, if fixing it is hard -- it's substantially better than the current situation.

> But I agree that conversion to HTML seems like a more ideal fix.

Yes -- let's see if we can find a victim to do that. :)
I played around a bit here, and have come up with this as an alternative hack to work around the problem -- WDYT? The use of setTimeout is particularly ugly, but I couldn't get it to behave without that. It seems to work OK in my local testing; the reflow is a bit laggy when resizing the dialog narrower, but that's not a big deal. Maybe worth considering, while we wait for an HTML rewrite?
Attachment #8683702 - Flags: feedback?(dholbert)
(In reply to Jonathan Kew (:jfkthame) from comment #30)
> the reflow is a bit laggy when resizing the
> dialog narrower

FWIW, that referred to testing with my usual debug build. I just tried it in an opt build, and AFAICT there's no perceptible impact there.
Comment on attachment 8683702 [details] [diff] [review]
Trigger an extra reflow when initially showing or resizing the Languages list in Preferences, to work around an XUL bug and ensure the languages are visible in RTL mode. f?dholbert

Seems fine to me, as a hackaround.

Please do include a comment explicitly mentioning that this is a hackaround for XUL bug 1194844, though (alongside the function and/or its invocations via onfocus/onresize).

(Otherwise, this is a bit magic & risks being accidentally cargo-culted during the HTML rewrite, or just misunderstood by humans reading the code.)
Attachment #8683702 - Flags: feedback?(dholbert) → feedback+
Workaround, now with extra comments. Obviously an HTML reimplementation is the right way forward, but in the meantime should we do something like this to fix the immediate user-visible problem?
Attachment #8683770 - Flags: review?(gijskruitbosch+bugs)
Attachment #8683702 - Attachment is obsolete: true
Comment on attachment 8683770 [details] [diff] [review]
Trigger an extra reflow when initially showing or resizing the Languages list in Preferences, to work around an XUL bug and ensure the languages are visible in RTL mode

Review of attachment 8683770 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/languages.js
@@ +20,5 @@
> +  // see bug 1194346.
> +  forceReflow: function ()
> +  {
> +    this._activeLanguages.style.fontKerning = "none";
> +    setTimeout("gLanguagesDialog._activeLanguages.style.fontKerning = 'normal'", 0);

I bet you can avoid the setTimeout if you force a style flush by reading getComputedStyle() or something similar. Can you check if that also works? I'm worried that the current approach would lead to races where it doesn't work.

Also, to ensure that if we ever change fontKerning for real, maybe the resetting line can be:

gLanguagesDialog._activeLanguages.style.removeProperty("font-kerning")

?
Attachment #8683770 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #34)

> I bet you can avoid the setTimeout if you force a style flush by reading
> getComputedStyle() or something similar. Can you check if that also works?

Seems like a possibility, but I'm afraid I'm not sure how to do that within the pref window. Trying something like:

  document.defaultView.getComputedStyle(this._activeLanguages)

just results in:

  TypeError: Argument 1 of Window.getComputedStyle does not implement interface Element

Any suggestions what I should be doing instead?

> Also, to ensure that if we ever change fontKerning for real, maybe the
> resetting line can be:
> 
> gLanguagesDialog._activeLanguages.style.removeProperty("font-kerning")

Sure, that ought to work too. (Though I picked that property largely because it seemed pretty unlikely we'd ever want to use it for real here.)
(In reply to Jonathan Kew (:jfkthame) from comment #35)
> (In reply to :Gijs Kruitbosch from comment #34)
> 
> > I bet you can avoid the setTimeout if you force a style flush by reading
> > getComputedStyle() or something similar. Can you check if that also works?
> 
> Seems like a possibility, but I'm afraid I'm not sure how to do that within
> the pref window. Trying something like:
> 
>   document.defaultView.getComputedStyle(this._activeLanguages)
> 
> just results in:
> 
>   TypeError: Argument 1 of Window.getComputedStyle does not implement
> interface Element


Err, that's very strange. Is this._activeLanguages maybe null initially? Though then the previous line shouldn't have worked either... Eval'ing "getComputedStyle(document.documentElement)" in the web console for about:preferences seems to work...

Anyway, reading clientHeight or whatever normally works to trigger a style flush, I believe?
(In reply to :Gijs Kruitbosch from comment #36)
> (In reply to Jonathan Kew (:jfkthame) from comment #35)
> > (In reply to :Gijs Kruitbosch from comment #34)
> > 
> > > I bet you can avoid the setTimeout if you force a style flush by reading
> > > getComputedStyle() or something similar. Can you check if that also works?
> > 
> > Seems like a possibility, but I'm afraid I'm not sure how to do that within
> > the pref window. Trying something like:
> > 
> >   document.defaultView.getComputedStyle(this._activeLanguages)
> > 
> > just results in:
> > 
> >   TypeError: Argument 1 of Window.getComputedStyle does not implement
> > interface Element
> 
> 
> Err, that's very strange. Is this._activeLanguages maybe null initially?
> Though then the previous line shouldn't have worked either... Eval'ing
> "getComputedStyle(document.documentElement)" in the web console for
> about:preferences seems to work...

OK, I'm not sure what I did wrong before, but I've now got getComputedStyle returning a CSS2Properties object successfully. However, that doesn't seem to be sufficient to force the reflow we need.

> Anyway, reading clientHeight or whatever normally works to trigger a style
> flush, I believe?

I thought that, but it seems to just be 'undefined' on the XULElement objects we're dealing with here. And given that calling getComputedStyle isn't sufficient, I suspect this is doomed too anyway.

So I'm afraid all I can offer is the setTimeout() version, which I realize looks hacky (though at least it's setTimeout(..., 0), so it isn't dependent on some specific interval) and perhaps not as robust as we'd wish. But for anything better, I think you need someone who actually understands the front-end code and how it interacts with XUL behavior and so on......
Updated to use removeProperty as suggested; marking r? again, as I can't get any of the 'better' suggestions to work.
Attachment #8684221 - Flags: review?(gijskruitbosch+bugs)
Attachment #8683770 - Attachment is obsolete: true
Comment on attachment 8684221 [details] [diff] [review]
Trigger an extra reflow when initially showing or resizing the Languages list in Preferences, to work around an XUL bug and ensure the languages are visible in RTL mode

Review of attachment 8684221 [details] [diff] [review]:
-----------------------------------------------------------------

Makes me sad, but OK.

Also, as regards the timeout parameter, IIRC we enforce minimum values of 4 or 10 or something depending on foreground-tab-ness and/or OS. But yeah, at least it's 0 in the code and not a magical "300ms should be enough" number.
Attachment #8684221 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #39)
> Makes me sad, but OK.

It doesn't make me happy either -- but if we replace it all with HTML goodness sometime soon, then at least the sadness will be short-lived. :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/92087a0c04b4ab5250325982b06dd6e71fcce489
Bug 1194346 - Trigger an extra reflow when initially showing or resizing the Languages list in Preferences, to work around an XUL bug and ensure the languages are visible in RTL mode. r=gijs
https://hg.mozilla.org/mozilla-central/rev/92087a0c04b4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(In reply to Carsten Book [:Tomcat] from comment #43)
> https://hg.mozilla.org/mozilla-central/rev/92087a0c04b4

Does this hack need to be executed on non RTL locales as well? If not, I'd suggest triggering the reflow only for the RTL locales, in order to lower the risk of potential regressions.
This hack needs to be executed if this chunk of XUL has "direction: rtl", which should only happen for RTL locales, but it could also happen if someone had a weird user stylesheet or has set the pref intl.uidirection.en = "rtl".

I'm not worried about comment 44; the risk of potential regressions here is low (one extra reflow isn't going to cause any trouble; there's an extremely minor perf cost, but that doesn't really matter for a single pane that's buried in the UI).  And if we made this hack RTL-specific, that'd require additional logic which I expect would introduce more room for human error (and potential regressions if we get the logic subtly wrong).
I believe it's only needed in RTL locales; but IMO the risk is sufficiently minimal that I'd prefer not to complicate things by adding extra code to check the locale (or its directionality).
Attached image issue.png
I've tested on Windows 7 64bit using Nightly 45.0a1 (buildID: 20151120030227) - ar, fa and he builds - and I have the following mention: after I've added a new language on the languages selection list, this entry is shifted out of view to the right (with APZ disabled). See screenshot "issue.png". The entry returns to normal (isn't shifted out anymore) after I scroll the list.

Any thoughts about this?
More precisely, all the entries are shifted (slightly) to the right, aren't they? That's what it looks like in your screenshot, at least.

In my (brief) testing, this occurs only in the case when there was no scrollbar in the list, and adding the new entry causes a scrollbar to appear. If you keep adding more entries once the scrollbar is already present, the problem doesn't happen. And if there were initially just a couple of entries, such that even after adding the new one, no scrollbar is needed, the problem doesn't happen.

Could you confirm this is the specific case where you're seeing the problem? I think we should be able to work around this fairly easily.

(Actually, it'd be best to file a followup bug, I think, so that we can keep track of the additional fix properly.)
Duplicate of this bug: 1246474
Blocks: 1472716
You need to log in before you can comment on or make changes to this bug.