1411241 - Result width is displayed different after maximizing the window

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Hani Yacoub, Desktop QA]

Attachment: result width.mp4

[Affected versions]: 
Nightly 58.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Open Firefox with a new profile.
2. Make sure to have some search history.
3. type g from google.com and observe the search results.
4. Minimize the window.
5. Type g from google.com and observe the search results.
6. Maximize the window.
7. Type g from google.com and observe the search results.

[Expected result]:
Results width should be displayed the same size even after maximizing the window.

[Actual result]:
Result width is displayed different after maximizing the window.

Comment 1

[:Paolo Amadini]

Thanks for finding this! In steps 5 and 7, I have to use the down arrow without typing, because changing the contents of the input field fixes the issue.

This bug existed before the changes in bug 1381427, with some results being truncated too early, but was less noticeable because there wasn't the border on the right side.

Comment 2

[Marco Bonardo [:mak]]

There seems to be some other issue shown in that video, most of the cut entries seem to be missing the ellipsis. Maybe a result from previously testing this same bug. I can't exclude these existed before the fix for bug 1381427, indeed it's likely most are just bugs that are a bit more visible now.

Comment 3

[Marco Bonardo [:mak]]

This surely needs polish.

Comment 4

[Hani Yacoub, Desktop QA]

Tested on Beta 57.0b11 and it's also affected.

Comment 5

[:Paolo Amadini]

This has the same root cause as the ellipsis disappearing in 57.0b10 and below. We set the new margins while the popup is closed, then we recalculate the overflow before opening it, but the measurements return incorrect values.

We can fix this by recalculating the overflow after the popup opens, but we'll have to whitelist additional reflows which will happen the first time the popup is opened or after the window is resized.

Comment 6

[:Paolo Amadini]

Attachment: Bug 1411241 - Result width is incorrect after maximizing the window.

We have to handle the item overflow after the popup opens in case the positioning has changed, otherwise the measurements would return incorrect values. This causes additional synchronous reflows the first time the popup is opened or after the window is resized.

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

Comment 7

[:Paolo Amadini]

Comment on attachment 8921902 [details]
Bug 1411241 - Result width is incorrect after maximizing the window.

Drew, also let us know if this matches your understanding, or you see better solutions that don't require a massive refactoring.

Comment 8

[:Paolo Amadini]

Also, my understanding is that we'll live with this for Firefox 57, because the bug only happens when reopening the same results just after resizing the window, it is always fixed the next time the popup is shown or after typing some characters, and the impact of having a different result alignment is minimal. Marco, is this correct?

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8921902 [details]
Bug 1411241 - Result width is incorrect after maximizing the window.

https://reviewboard.mozilla.org/r/192936/#review198320

Makes sense I think.  I don't see any better solutions, even ones that do require massive refactoring!

> but the measurements return incorrect values.

Exactly because the popup is closed, right?  They don't become right until it's opened.

::: browser/base/content/urlbarBindings.xml:1918
(Diff revision 1)
>              let identityRect =
>                this.DOMWindowUtils.getBoundsWithoutFlushing(identityIcon);
> -            if (popupDirection == "rtl") {
> -              this.margins = { start: documentRect.right - identityRect.right,
> -                               end: endOffset };
> -            } else {
> +            let start = popupDirection == "rtl" ?
> +                        documentRect.right - identityRect.right :
> +                        identityRect.left;
> +            if (!this._margins || start != this.margins.start ||

The second margins access on this line isn't underscored, unlike the rest of them.  Not that it practically matters of course.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8921902 [details]
Bug 1411241 - Result width is incorrect after maximizing the window.

https://reviewboard.mozilla.org/r/192936/#review198504

Please check Drew's comments too.
This looks ok to me, we still have bug 1356532 open for reflows. I still think we should really re-evaluate our approach, maybe in the end we should really 2-column the matches.

::: browser/base/content/urlbarBindings.xml:1970
(Diff revision 1)
>            this.openPopup(aElement, "after_start", 0, yOffset, false, false);
> +
> +          // Do this immediately after we've requested the popup to open. This
> +          // will cause sync reflows but prevents flickering.
> +          if (needsHandleOverUnderflow) {
> +            for (let item of this.richlistbox.childNodes) {

could we wrap this loop in requestAnimationFrame? Mostly asking if you could test whether doing that would still solve this bug, since I expect it to be a bit cheaper.

Comment 11

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #8)
> Also, my understanding is that we'll live with this for Firefox 57, because
> the bug only happens when reopening the same results just after resizing the
> window, it is always fixed the next time the popup is shown or after typing
> some characters, and the impact of having a different result alignment is
> minimal. Marco, is this correct?

it's correct, yes.

Comment 12

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #10)
> could we wrap this loop in requestAnimationFrame? Mostly asking if you could
> test whether doing that would still solve this bug, since I expect it to be
> a bit cheaper.

I'm wary of doing that, lest we introduce more flickering...

Comment 13

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aec836ffdf01d106f1e8b9aa16d4a01f21efef5

Comment 14

[:Paolo Amadini]

Comment on attachment 8921902 [details]
Bug 1411241 - Result width is incorrect after maximizing the window.

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

Comment 15

[:Paolo Amadini]

Had to whitelist another test. New tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9616d16f51e5d5a9521e4437464d09dc02dffc53

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44719d79475
Result width is incorrect after maximizing the window. r=mak

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/f44719d79475

Comment 18

[Hani Yacoub, Desktop QA]

Build ID: 20171029220112
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.