Bugzilla

Comment 2

•

8 years ago

Thanks -- I'll take a look tomorrow.

Comment 3

•

8 years ago

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.)

Comment 4

•

8 years ago

Attached file testcase 1 — Details

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.

Comment 5

•

8 years ago

Attached video screencast of bug in Nightly, using testcase 1 — Details

Here's a screencast of the bug, with testcase 1, in standard Nightly. (no need for any particular localization)

Flags: needinfo?(dholbert)

Comment 6

•

8 years ago

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.)

Alice0775 White

Comment 7

•

8 years ago

Attached video screencast_apz_enabled.ogg — Details

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.

Comment 8

•

8 years ago

(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.

Updated

•

8 years ago

Depends on: 1194844

Comment 9

•

8 years ago

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).

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Updated

•

8 years ago

Component: Layout: Text → Preferences

Product: Core → Firefox

Alice0775 White

Comment 10

•

8 years ago

(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

Comment 11

•

8 years ago

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?

tracking-firefox43: ? → +

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Comment 12

•

8 years ago

Tracked for FF41 and 42.

tracking-firefox41: ? → +

tracking-firefox42: ? → +

Sylvestre Ledru [:Sylvestre]

Comment 13

•

8 years ago

Too late for 40 and this bug won't be fixed in ESR (we only take security fixes and top critical issues).

status-firefox40: affected → wontfix

status-firefox-esr38: affected → wontfix

tracking-firefox40: ? → +

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Comment 14

•

8 years ago

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.

status-firefox41: affected → wontfix

Sylvestre Ledru [:Sylvestre]

Comment 15

•

8 years ago

We published (at least) two releases with this bug, no duplicate, not a big deal, untracking?

tracking-firefox42: + → -

tracking-firefox43: + → -

Comment 17

•

8 years ago

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>

Comment 18

•

8 years ago

(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

Comment 19

•

8 years ago

Attached patch hackaround patch v1: force this listbox to be LTR — Details — Splinter Review

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)

Comment 20

•

8 years ago

Attached image — Details

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).

Comment 21

•

8 years ago

Attached image — Details

...and here's a screenshot with the patch (again, with intl.uidirection.en set to rtl)

Comment 22

•

8 years ago

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 23

•

8 years ago

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-

Comment 24

•

8 years ago

:gijs, can you take this bug, or know someone else who can work on it?

Flags: needinfo?(gijskruitbosch+bugs)

Comment 25

•

8 years ago

(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)

Comment 26

•

8 years ago

(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.

Comment 27

•

8 years ago

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).

Comment 28

•

8 years ago

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.

Comment 29

•

8 years ago

(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. :)

Comment 30

•

8 years ago

Attached patch 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 (obsolete) — Details — Splinter Review

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)

Comment 31

•

8 years ago

(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 32

•

8 years ago

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+

Comment 33

•

8 years ago

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)

Updated

•

8 years ago

Attachment #8683702 - Attachment is obsolete: true

Comment 34

•

8 years ago

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+

Comment 35

•

8 years ago

(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.)

Comment 36

•

8 years ago

(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?

Comment 37

•

8 years ago

(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......

Comment 38

•

8 years ago

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)

Updated

•

8 years ago

Attachment #8683770 - Attachment is obsolete: true

Comment 39

•

8 years ago

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+

Comment 40

•

8 years ago

(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. :)

Comment 41

•

8 years ago

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

Carsten Book [:Tomcat]

Comment 42

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/92087a0c04b4

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox45: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 45

Carsten Book [:Tomcat]

Comment 43

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/92087a0c04b4

Tomer Cohen :tomer

Comment 44

•

8 years ago

(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.

Comment 45

•

8 years ago

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).

Camelia Badau [:cbadau], Release Desktop QA

Comment 46

•

8 years ago

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).

Comment 47

•

8 years ago

Attached image — Details

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?