1369729 - Synced tabs view gets cut off when device descriptions are multiline (needs descriptionheightworkaround)

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[:Gijs (he/him)]

http://imgur.com/a/gAUvW

Though we create a scrollbar, the cut-off is presumably because we don't run the descriptionheightworkaround .

Comment 3

[:Gijs (he/him)]

Paolo, it seems like this attribute lives on the panelmultiview right now. I don't really understand why that is, because it seems to me that it should live on the view nodes that require this, as not every view node in each of these panelmultiviews actually requires us to run this code... can we move the attribute? Am I missing something?

(In particular, in this case I think we need it for the remote tabs view, which can be shown in the temporary (photon)panelmultiview we create in panelUI.js for the library button, but I don't really want it run on *every* temporary panel...)

Comment 4

[:Paolo Amadini]

Yes, the attribute can live on the view nodes, that are already conveniently passed to the workaround function. When I started implementing this I thought this would only be required by the non-Photon panels, so I put the attribute there, but the individual view is a better place.

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1369729 - use descriptionheightworkaround for sync panel,

Review commit: https://reviewboard.mozilla.org/r/150496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150496/

Comment 6

[:Gijs (he/him)]

Please check that I actually added this attribute to all the views that need it. I *think* I did, but I don't know the downloads panel that well. :-)

Comment 7

[:Paolo Amadini]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

https://reviewboard.mozilla.org/r/150496/#review155734

::: browser/components/customizableui/PanelMultiView.jsm:1039
(Diff revision 1)
>      // this is called, and in the best case scenario, this has no effect.
>      let items = [];
> -    for (let element of viewNode.querySelectorAll(
> -         "description:not([hidden]):not([value]),toolbarbutton[wrap]:not([hidden])")) {
> +    let selector = [
> +      "description:not([hidden]):not([value])",
> +      "toolbarbutton[wrap]:not([hidden])",
> +      "label:not(:-moz-any([hidden],[value],:empty))"

nit: comma at end of line

I'm not sure about how ":not(:-moz-any" works, but it seems that if we stop discriminating between "label" and "description" and use the attributes instead, the selectors we use should be the same...

Are there any multiline elements that are legitimately "label" (i.e. they target another control) and as such shouldn't be converted to "description" instead?

::: browser/components/customizableui/content/panelUI.inc.xul:14
(Diff revision 1)
>         flip="slide"
>         position="bottomcenter topright"
>         noautofocus="true">
>    <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView"
>                    viewCacheId="appMenu-viewCache">
>      <panelview id="PanelUI-mainView" context="customizationPanelContextMenu">

The main non-Photon view needs the workaround for the update badge.

::: browser/components/customizableui/content/panelUI.inc.xul:302
(Diff revision 1)
>                  class="PanelUI-characterEncodingView-list"/>
>          </vbox>
>        </vbox>
>      </panelview>
>  
>      <panelview id="PanelUI-panicView" flex="1">

It looks like this view has some multi-line elements. They should be "description" rather than "label" elements.

::: browser/components/downloads/content/downloadsOverlay.xul:112
(Diff revision 1)
> -        <panelview id="downloadsPanel-mainView">
> +        <panelview id="downloadsPanel-mainView"
> +                   descriptionheightworkaround="true">

We actually don't need the workaround for the main view of the Downloads Panel, because the description element is not multiline anymore.

Comment 8

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #7)
> Are there any multiline elements that are legitimately "label" (i.e. they
> target another control) and as such shouldn't be converted to "description"
> instead?

Maybe, but this feels like it has potentially much wider impact - we'd need to update styling and so on, and we would probably affect non-photon as well. Can we not block this bug on fixing this (ie leave things that are <label> as <label> for now) and deal with this issue in a follow-up? I don't think it's directly related to what we are trying to fix here (ie the synced tab view will still need the magic attribute, and switching to <description> by itself won't make anything better in terms of layout, even if it's semantically more correct).

Comment 9

[:Paolo Amadini]

Given the possible styling implications, I'm fine with doing this in a follow-up, or even keeping the elements as they are. The downside is that we're probably scanning more elements than necessary, on the other hand it seems the selectors should already exclude the most common cases of single-line labels, so we're not doing much extra work.

I believe the description of the function might need some fine tuning.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150496/diff/1-2/

Comment 11

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150496/diff/2-3/

Comment 12

[:Paolo Amadini]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

https://reviewboard.mozilla.org/r/150496/#review157062

Comment 13

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

https://reviewboard.mozilla.org/r/150496/#review157066

::: browser/components/customizableui/PanelMultiView.jsm:1036
(Diff revision 3)
> -    for (let element of viewNode.querySelectorAll(
> -         "description:not([hidden]):not([value]),toolbarbutton[wrap]:not([hidden])")) {
> +    // Non-hidden <label> or <description> elements that also aren't empty
> +    // and also don't contain a value attribute can be multiline (if their
> +    // text content is long enough.

Looks like I missed a closing brace in this comment here, and s/contain/has/ would be ever so slightly more readable. Will fix before landing.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150496/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0772946156f
use descriptionheightworkaround for sync panel, r=Paolo

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/391c04c22f93
Backed out changeset c0772946156f for failing browser-chrome's browser_appmenu_reflows.js with unexpected uninterruptible reflow. r=backout

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser_appmenu_reflows.js with unexpected uninterruptible reflow:

https://hg.mozilla.org/integration/autoland/rev/391c04c22f939624a30969f1a43cc7e94ad62920

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c0772946156f06a6d8fd6748a4f28b14b6855181&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=109507722&repo=autoland

[task 2017-06-23T09:29:11.009145Z] 09:29:11     INFO - TEST-PASS | browser/base/content/test/performance/browser_appmenu_reflows.js | expected uninterruptible reflow: '[
[task 2017-06-23T09:29:11.010894Z] 09:29:11     INFO - 	"get_alignmentPosition@chrome://global/content/bindings/popup.xml:158:11",
[task 2017-06-23T09:29:11.012888Z] 09:29:11     INFO - 	"adjustArrowPosition@chrome://global/content/bindings/popup.xml:428:13",
[task 2017-06-23T09:29:11.014708Z] 09:29:11     INFO - 	"onxblpopuppositioned@chrome://global/content/bindings/popup.xml:525:9",
[task 2017-06-23T09:29:11.016478Z] 09:29:11     INFO - 	""
[task 2017-06-23T09:29:11.019212Z] 09:29:11     INFO - ]' - true == true - 
[task 2017-06-23T09:29:11.020969Z] 09:29:11     INFO - Buffered messages finished
[task 2017-06-23T09:29:11.023388Z] 09:29:11     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu_reflows.js | unexpected uninterruptible reflow 
[task 2017-06-23T09:29:11.025882Z] 09:29:11     INFO - [
[task 2017-06-23T09:29:11.029018Z] 09:29:11     INFO - 	"descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm:1053:20",
[task 2017-06-23T09:29:11.030911Z] 09:29:11     INFO - 	"onTransitionEnd@resource:///modules/PanelMultiView.jsm:521:11",
[task 2017-06-23T09:29:11.032998Z] 09:29:11     INFO - 	"listener@resource:///modules/PanelMultiView.jsm:597:15",
[task 2017-06-23T09:29:11.034836Z] 09:29:11     INFO - 	"EventListener.handleEvent*showSubView/</<@resource:///modules/PanelMultiView.jsm:589:13",
[task 2017-06-23T09:29:11.036901Z] 09:29:11     INFO - 	"EventListener.handleEvent*showSubView/<@resource:///modules/PanelMultiView.jsm:571:11",
[task 2017-06-23T09:29:11.039010Z] 09:29:11     INFO - 	"_viewBoundsOffscreen/<@resource:///modules/PanelMultiView.jsm:674:7",
[task 2017-06-23T09:29:11.041878Z] 09:29:11     INFO - 	"EventListener.handleEvent*_viewBoundsOffscreen@resource:///modules/PanelMultiView.jsm:665:5",
[task 2017-06-23T09:29:11.043676Z] 09:29:11     INFO - 	"showSubView/<@resource:///modules/PanelMultiView.jsm:543:9",
[task 2017-06-23T09:29:11.046109Z] 09:29:11     INFO - 	"async*showSubView@resource:///modules/PanelMultiView.jsm:406:13",
[task 2017-06-23T09:29:11.048488Z] 09:29:11     INFO - 	"value@resource:///modules/PanelMultiView.jsm:295:42",
[task 2017-06-23T09:29:11.051070Z] 09:29:11     INFO - 	"showSubView@chrome://browser/content/customizableui/panelUI.js:492:7",
[task 2017-06-23T09:29:11.052961Z] 09:29:11     INFO - 	"async*oncommand@chrome://browser/content/browser.xul:1:1",
[task 2017-06-23T09:29:11.055293Z] 09:29:11     INFO - 	"openSubViewsRecursively@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:113:9",
[task 2017-06-23T09:29:11.058548Z] 09:29:11     INFO - 	"async*openSubViewsRecursively@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:118:15",
[task 2017-06-23T09:29:11.060526Z] 09:29:11     INFO - 	"async*@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:124:11",
[task 2017-06-23T09:29:11.062464Z] 09:29:11     INFO - 	"async*withReflowObserver@chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:139:11",
[task 2017-06-23T09:29:11.064660Z] 09:29:11     INFO - 	"async*@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_appmenu_reflows.js:100:9",
[task 2017-06-23T09:29:11.067291Z] 09:29:11     INFO - 	"Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:779:21",
[task 2017-06-23T09:29:11.069117Z] 09:29:11     INFO - 	"TaskImpl_run@resource://gre/modules/Task.jsm:321:42",
[task 2017-06-23T09:29:11.072184Z] 09:29:11     INFO - 	"TaskImpl@resource://gre/modules/Task.jsm:279:3",
[task 2017-06-23T09:29:11.074150Z] 09:29:11     INFO - 	"asyncFunction@resource://gre/modules/Task.jsm:254:14",
[task 2017-06-23T09:29:11.077097Z] 09:29:11     INFO - 	"Task_spawn@resource://gre/modules/Task.jsm:168:12",
[task 2017-06-23T09:29:11.079053Z] 09:29:11     INFO - 	"Tester_execTest@chrome://mochikit/content/browser-test.js:774:9",
[task 2017-06-23T09:29:11.080925Z] 09:29:11     INFO - 	"Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:686:7",
[task 2017-06-23T09:29:11.083695Z] 09:29:11     INFO - 	"SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59",
[task 2017-06-23T09:29:11.085394Z] 09:29:11     INFO - 	""
[task 2017-06-23T09:29:11.087037Z] 09:29:11     INFO - ]
[task 2017-06-23T09:29:11.090812Z] 09:29:11     INFO -  - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reflow :: line 117
[task 2017-06-23T09:29:11.092512Z] 09:29:11     INFO - Stack trace:
[task 2017-06-23T09:29:11.095060Z] 09:29:11     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reflow:117
[task 2017-06-23T09:29:11.096780Z] 09:29:11     INFO - resource:///modules/PanelMultiView.jsm:descriptionHeightWorkaround:1053
[task 2017-06-23T09:29:11.098593Z] 09:29:11     INFO - resource:///modules/PanelMultiView.jsm:onTransitionEnd:521
[task 2017-06-23T09:29:11.100786Z] 09:29:11     INFO - resource:///modules/PanelMultiView.jsm:listener:597
[task 2017-06-23T09:29:11.103025Z] 09:29:11     INFO - Not taking screenshot here: see the one that was previously logged

Comment 18

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150496/diff/4-5/

Comment 19

[:Gijs (he/him)]

Relanding with the expectations of that test updated. It's expected (and, sadly, unavoidable) that this change adds sync reflows.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6d7647024de
use descriptionheightworkaround for sync panel, r=Paolo

Comment 21

[:Gijs (he/him)]

This is about to be backed out a second time, because the appmenu reflow test requires a deterministic number of reflows, and the dirtying of the frame tree done by the test seems to sometimes cause a second reflow and sometimes not, so the number of reflows is not predictable, so the result is that the test is either intermittent orange for unexpected reflows, or for 'missing' expected reflows. I don't know how to fix this. Do you have suggestions?

Comment 22

[Carsten Book [:Tomcat]]

backout for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=109552639&repo=autoland

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04f5b18e6f40
Backed out changeset a6d7647024de for reflow issues

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

https://reviewboard.mozilla.org/r/150496/#review157540

Do we know where this non-determinism is coming from? What is sometimes dirtying the frame tree, and sometimes not? Is it because of something like a setTimeout, requestAnimationFrame or requestIdleCallback?

::: browser/base/content/test/performance/browser_appmenu_reflows.js:79
(Diff revision 5)
> -   * Nothing here! Please don't add anything new!
> +   * The synced tabs view has labels that are multiline. Because of bugs in
> +   * XUL layout relating to multiline text in scrollable containers, we need
> +   * to manually read their height in order to ensure container heights are
> +   * correct. Unfortunately this requires 2 sync reflows.
> +   *
> +   * If we add more views where this is necessary, we may need to duplicate
> +   * these expected reflows further.
> +   */
> +  [
> +    "descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm",
> +    "onTransitionEnd@resource:///modules/PanelMultiView.jsm",
> +  ],
> +  [
> +    "descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm",
> +    "onTransitionEnd@resource:///modules/PanelMultiView.jsm",
> +  ],

Yikes - we should definitely not be adding to this list, ever. We need to find a different approach here.

Comment 25

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #24)
> ::: browser/base/content/test/performance/browser_appmenu_reflows.js:79
> (Diff revision 5)
> > -   * Nothing here! Please don't add anything new!
> > +   * The synced tabs view has labels that are multiline. Because of bugs in
> > +   * XUL layout relating to multiline text in scrollable containers, we need
> > +   * to manually read their height in order to ensure container heights are
> > +   * correct. Unfortunately this requires 2 sync reflows.
> > +   *
> > +   * If we add more views where this is necessary, we may need to duplicate
> > +   * these expected reflows further.
> > +   */
> > +  [
> > +    "descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm",
> > +    "onTransitionEnd@resource:///modules/PanelMultiView.jsm",
> > +  ],
> > +  [
> > +    "descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm",
> > +    "onTransitionEnd@resource:///modules/PanelMultiView.jsm",
> > +  ],
> 
> Yikes - we should definitely not be adding to this list, ever. We need to
> find a different approach here.

I disagree. We're basically been doing layout's job here, and have been doing it for a while, because it isn't doing it properly itself, and platform folks apparently don't have time to do this. There is no way to do this lazily without causing flickering. We are *already* doing this, it just doesn't get hit in this particular test. It's not OK to block this patch just because it now trips some automated checks that should have been tripped all along.

Comment 26

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #24)
> Comment on attachment 8879163 [details]
> Bug 1369729 - use descriptionheightworkaround for sync panel,
> 
> https://reviewboard.mozilla.org/r/150496/#review157540
> 
> Do we know where this non-determinism is coming from? What is sometimes
> dirtying the frame tree, and sometimes not? Is it because of something like
> a setTimeout, requestAnimationFrame or requestIdleCallback?

This code runs a loop of height readings:

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1036-1041

it doesn't dirty the frame tree, so it will really only hit once, even if there are multiple elements in the loop, but on infra this is hitting multiple times. Florian said this is because the test expressly tries to dirty the frame tree after every sync flush, which is presumably why we're hitting this. If that is right, and that itself is not deterministic, that's an issue with this test, not with the code under test, and I don't know how to fix that.

Comment 27

[:Gijs (he/him)]

Mike tried a few things - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c9712156258a4299f9721bb7a015e9f97529770&selectedJob=109911154 - unfortunately is still orange on try (though now with 'Unused expected reflow' instead).

Other guesses I can come up with here include that somehow, we're hitting the other path into this code outside of the test (ie https://hg.mozilla.org/try/rev/8a34558942bad2400f1c8fbfde76b56c01b314a7#l3.12 ) and thus cache whatever info we need and then don't re-run later. Or that the number of elements we find in the different runs that we apply the workaround to somehow varies (though it's not clear to me how/why that would be the case). Or finally that there's something odd about how we dirty/optimize layout state that breaks this.

Comment 28

[:Paolo Amadini]

For what it's worth, this is the issue I anticipated in bug 1369339 comment 23. I agree with Gijs in comment 25 that we have to live with this situation until bug 1293242 is fixed.

Does the test already run independently from all other test, or is there a chance that other test code runs before it? This might be one reason for which the result is non-deterministic.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150496/diff/5-6/

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

https://reviewboard.mozilla.org/r/150496/#review159346

Sorry for the delay! Thanks for not disabling the test entirely. Hopefully we can get this thing turned back on soon once I get it to behave.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b6c709d2348
use descriptionheightworkaround for sync panel, r=mconley,Paolo

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b6c709d2348

Comment 33

[:Paolo Amadini]

If you plan to uplift this, it's slightly easier to do it before uplifting bug 1377793.

Comment 34

[:Gijs (he/him)]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

Approval Request Comment
[Feature/Bug causing the regression]: not super clear. This became more obvious with the library panel (which we're not shipping in 55) but I think it will also affect the synced tabs view in the hamburger panel after bug 1009116, as well as fixing some forget button issues that had been outstanding for a lot longer.
[User impact if declined]: cut-off UI
[Is this code covered by automated tests?]: partially - we do some testing about the display of panels for webextensions and the main menu panel, as well as reflow testing, but the actual visual appearance of the panel is not tested automatically.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:

1. sign into sync on an account that has exactly 1 other Firefox instance connected to it. Make sure the other instance's Firefox sync device name is more than 30 or so characters, with some spaces inbetween
2. in the other instance, open approximately 10 non-blank (ie not "new tab") tabs
3. open the 'synced tabs' item in the main hamburger panel (either non-photon, or in photon, via the library), with Firefox at the top of the screen. The number of tabs should be such that the panel expands, but doesn't get a scrollbar

ER:
no scrollbar, all tabs fit

before this patch:
either a scrollbar or the tabs don't fit

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it mostly moves some existing code around and ensures it applies to the synced tab view as well as the forget button. We also still have 1 month of beta left.
[String changes made/needed]: nope

Comment 35

[Julien Cristau [:jcristau]]

Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

fix for sync panel visual issue, beta55+

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e56241dd21ae

Comment 37

[Kanchan Kumari QA]

I verified this bug fix on Fx 55, Fx 56 and the latest nightly. There is no scrollbar for ~12 tabs and all tabs fit in synced tabs panel. 

On a side note, I do see extra spaces (Vertically) in the panel with Fx 55 and 56 which seems fixed in Fx 57.