Closed Bug 1369729 Opened 3 years ago Closed 3 years ago

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

Categories

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

53 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Status
firefox55 --- verified
firefox56 --- verified
firefox57 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

http://imgur.com/a/gAUvW

Though we create a scrollbar, the cut-off is presumably because we don't run the descriptionheightworkaround .
Duplicate of this bug: 1370078
Summary: Synced tabs view needs descriptionheightworkaround → Synced tabs view gets cut off when device descriptions are multiline (needs descriptionheightworkaround)
Flags: qe-verify+
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Duplicate of this bug: 1371025
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Depends on: 1369095
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...)
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
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 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.
Attachment #8879163 - Flags: review?(paolo.mozmail)
(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).
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
Blocks: 1088003
See Also: → 1375143
Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

https://reviewboard.mozilla.org/r/150496/#review157062
Attachment #8879163 - Flags: review?(paolo.mozmail) → review+
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.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c0772946156f
use descriptionheightworkaround for sync panel, r=Paolo
Backout by archaeopteryx@coole-files.de:
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
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
Flags: needinfo?(gijskruitbosch+bugs)
Relanding with the expectations of that test updated. It's expected (and, sadly, unavoidable) that this change adds sync reflows.
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a6d7647024de
use descriptionheightworkaround for sync panel, r=Paolo
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?
Flags: needinfo?(mconley)
Flags: needinfo?(florian)
backout for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=109552639&repo=autoland
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04f5b18e6f40
Backed out changeset a6d7647024de for reflow issues
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.
Attachment #8879163 - Flags: review-
Flags: needinfo?(mconley)
(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.
(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.
Flags: needinfo?(mconley)
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.
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.
Depends on: 1293242
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Blocks: 1376822
Depends on: 1377716
Blocks: 1377716
No longer depends on: 1377716
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.
Attachment #8879163 - Flags: review?(mconley) → review+
Flags: needinfo?(mconley)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8b6c709d2348
use descriptionheightworkaround for sync panel, r=mconley,Paolo
https://hg.mozilla.org/mozilla-central/rev/8b6c709d2348
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
If you plan to uplift this, it's slightly easier to do it before uplifting bug 1377793.
Flags: needinfo?(florian) → needinfo?(gijskruitbosch+bugs)
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8879163 - Flags: approval-mozilla-beta?
Comment on attachment 8879163 [details]
Bug 1369729 - use descriptionheightworkaround for sync panel,

fix for sync panel visual issue, beta55+
Attachment #8879163 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1387512
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.