1480349 - If the browser window is resized the real host (سماء.شبكة) is not visible

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[ovidiu boca[:Ovidiu]]

This is a follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=1419391#c47 the second issue.

[Affected versions]:

Tested on Nightly 63.0a1(2018-08-01)

[Affected platforms]:

Mac OS, Linux, Windows

STR:

1. Open a Firefox window in full screen and go to https://xn--ggbla1c4e.xn--ngbc5azd/#%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%D7%A1%D7%95%D6%B9.%D7%A1%D7%97

2. Resize the browser window

ER: The real host (سماء.شبكة) is visible if the browser window is resized. 

AR: The real host (سماء.شبكة) is not visible if the browser window is resized, only in full screen. 

Note: Here is a recording with the issue: https://streamable.com/7iein

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1480349 - RTL hosts don't stay visible when the browser window is resized.

On window resize we should format the urlbar again, to ensure the host stays visible.
Also fixes bug 1192501, that requires the same resize handler.

Comment 2

[Phabricator Automation]

Comment on attachment 8997374 [details]
Bug 1480349 - RTL hosts don't stay visible when the browser window is resized.

Drew Willcoxon :adw has approved the revision.

https://phabricator.services.mozilla.com/D2711

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dcb573bd271
RTL hosts don't stay visible when the browser window is resized. r=adw

Comment 4

[Noemi Erli[:noemi_erli]]

Backed out changeset 2dcb573bd271 (Bug 1480349) for failures in rowser/base/content/test/urlbar/browser_urlbarAddonIframe.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2dcb573bd271c375001a5a892d9b734c66e51a80&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=192511195

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=192511195&repo=autoland&lineNumber=6607

Backout: https://hg.mozilla.org/integration/autoland/rev/b0703897aa8a89549a1cdf4154e2d19ca0021778

Comment 5

[Marco Bonardo [:mak]]

it looks like an intermittent to me
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2dcb573bd271c375001a5a892d9b734c66e51a80
(plus bug 1330506)
Anyway, I think I can probably make it a bit more stable.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5ba45e6ac73
RTL hosts don't stay visible when the browser window is resized. r=adw

Comment 7

[Cristian Brindusan [:cbrindusan]]

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

Comment 8

[Andreea Pavel [:apavel]]

Backed out on request

Backout: https://hg.mozilla.org/mozilla-central/rev/056a3c3fcc42cf5e22bfa979b0390b3d88d83b65

Comment 9

[Drew Willcoxon :adw]

To fix/prevent the talos regression, I think we could just call formatValue and do the scrolling when the resize stops.  i.e., if the timeout already exists, cancel it and restart it.  The patch here did that continuously with the resize, even though it throttled it.

Comment 10

[Drew Willcoxon :adw]

Attachment: Bug 1480349 - RTL hosts don't stay visible when the browser window is resized.

This does a few things that should help:

(1) Run the timeout only after resize events have stopped.

(2) Increase the timeout from 30ms to 100ms.  It doesn't need to be so fast.

(3) Add a parameter to formatValue that bypasses actually formatting the value and instead only ensures the host is visible.  There's no need in this case to keep removing the formatting and then adding it back.  Having a "formatValue" method take a parameter to skip formatting is kind of weird, though.  At first I tried factoring out the make-host-visible part, but that requires also factoring out all the logic that determines whether the value is a formattable URL, and that ended up being ugly.  I'm open to better ideas.

Based on an earlier patch by Marco Bonardo <[e-mail]>

Comment 11

[Drew Willcoxon :adw]

I'll ask for review once the talos results come in.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6209dd7a2cef0ebfdbeceeb3fc8909478be918d9

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=6209dd7a2cef0ebfdbeceeb3fc8909478be918d9

Comment 12

[Drew Willcoxon :adw]

Hope it's OK if I steal this, Marco.  I had wanted to get a patch while you were on PTO.

Comment 13

[Drew Willcoxon :adw]

Talos looks OK I think, no red flashing lights...

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=3205543f957cd2a6905486d73aa897535fdd9825&newProject=try&newRevision=6209dd7a2cef0ebfdbeceeb3fc8909478be918d9&framework=1

This compares the patch to the patch's parent revision.  The confidence in the tresize deltas is "low" in every case.  In 3 cases the patch showed a slight improvement, and in 4 cases a slight regression.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=6209dd7a2cef0ebfdbeceeb3fc8909478be918d9&framework=1&selectedTimeRange=172800

This uses the default baseline: "By default, Perfherder will compare against performance data gathered over the last 2 days from when new revision was pushed."  In this case, the confidence is still "low" in every case, but each case is a regression, and the biggest is 6.06%.

Comment 14

[Drew Willcoxon :adw]

I pushed Marco's original patch to try to compare how it performs under the same conditions:

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=465fc4e4341e0b5e834c9ea59e79a2b9ba2a7291

Comment 15

[Drew Willcoxon :adw]

The first time I ran this, I didn't do any retriggers, so I ran it again with 5 retriggers, as the trychooser page suggests.

(1) Original patch, baseline = last 2 days of m-c:

No significant regressions, but a med-confidence 1.61% regression on macOS -- https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=356a7e8835ded80137702cf6164c77de0eb06510&framework=1&showOnlyComparable=1&selectedTimeRange=172800

(2) New patch, baseline = last 2 days of m-c:

No significant regressions, and actually a very small low-confidence improvement on macOS -- https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=7a79721276344014b9407032a1f0303ad4ba8e9e&framework=1&showOnlyComparable=1&selectedTimeRange=172800

(3) Original patch, baseline = patch parent:

Significant/yellow med-confidence regression of 2.53% on macOS (almost same as bug 1481837), smaller low-confidence regression on a Windows machine -- https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=3205543f957cd2a6905486d73aa897535fdd9825&newProject=try&newRevision=356a7e8835ded80137702cf6164c77de0eb06510&framework=1&showOnlyComparable=1

(4) New patch, baseline = patch parent:

No significant regressions, but a similar low-confidence small regression on a Windows machine -- https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=3205543f957cd2a6905486d73aa897535fdd9825&newProject=try&newRevision=7a79721276344014b9407032a1f0303ad4ba8e9e&framework=1&showOnlyComparable=1

* * *

And actually in both cases (1) and (2) there was a red 3.65% regression in "about_preferences_basic pgo e10s stylo".  That's not present in cases (3) and (4) so I'm hoping that's something else in my tree, or something.

I'll go ahead and ask for review since it seems like the ~2.5% regression from case (3) is fixed in (4), by the new patch.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 9004742 [details]
Bug 1480349 - RTL hosts don't stay visible when the browser window is resized.

Marco Bonardo [::mak] has approved the revision.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76ccb62dd14d
RTL hosts don't stay visible when the browser window is resized. r=mak

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/76ccb62dd14d

Comment 19

[Eliza Balazs [:ebalazs_]]

Backed out changeset 76ccb62dd14d (Bug 1480349) for bc failures on browser_urlbar_search.js a=backout

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=76ccb62dd14d57f1fdb8e9448b97eb06500ba9d7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-searchStr=windows%20bc&selectedJob=196606068

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=815b46ed118286d682ce258e6147df5dc691eabf&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 20

[Drew Willcoxon :adw]

I can't get this to fail locally, going to take some time to debug

Comment 21

[Drew Willcoxon :adw]

Looks like closing the popup in the resize handler is the problem, which I think was a ride-along to fix bug 1192501: https://hg.mozilla.org/mozilla-central/rev/76ccb62dd14d#l2.146

But that shouldn't make the test fail, so I'll try to figure out what's going on.

Comment 22

[Drew Willcoxon :adw]

Moving closePopup() outside the timeout seems to fix it.  I guess the timeout happens to be firing in this test after it opens the popup and expects it to be open.  Makes sense.  If we kept it inside the timeout, we'd need to make sure that if the popup is open in the timeout, it was open when the timeout was set and it wasn't closed and reopened in the meantime.

I'll land this without asking for review on that change since it's not so big.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2dcf1b6dd09
RTL hosts don't stay visible when the browser window is resized. r=mak

Comment 24

[Eliza Balazs [:ebalazs_]]

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

Comment 25

[ovidiu boca[:Ovidiu]]

I tested this issue with the latest Nightly 64.0a1 (2018-09-05) and I found out a potential issue: 

STR:  double-click on the tab bar to resize the browser windows everything works as expected and the real host is displayed, but if the URL bar is selected or the caret is in the URL bar and you resize the browser windows(double-click on the tab bar or clicking on the resize buttons) you will see that the real host is not displayed anymore. 

Here is a screen recording with the issue: https://streamable.com/1x25j

Drew, please take a look at this and tell me your opinion.

Comment 26

[Drew Willcoxon :adw]

It's expected that no scrolling happens when the urlbar has focus, and I don't think this bug was supposed to address that.  I'm not sure we want to scroll at all while the urlbar is focued because it's potentially disruptive.

Comment 27

[ovidiu boca[:Ovidiu]]

Thanks, Drew for your answer, so from your comment 26, I see that this is the expected behavior. I will mark this as verified fixed. 
I tested this on Mac Os X 10.12, Windows 10 and Ubuntu 16.04 with FF Beta 63 and FF Nightly 64.0a1(2018-09-06) and I can confirm the fix.