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

VERIFIED FIXED in Firefox 63

Status

()

defect
P1
normal
VERIFIED FIXED
9 months ago
8 months ago

People

(Reporter: Ovidiu, Assigned: adw)

Tracking

Trunk
Firefox 63
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified, firefox64 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

(Reporter)

Description

9 months ago
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
(Reporter)

Updated

9 months ago
Blocks: 1419391
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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 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
Attachment #8997374 - Flags: review+

Comment 3

9 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/2dcb573bd271
RTL hosts don't stay visible when the browser window is resized. r=adw
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.
Flags: needinfo?(mak77)

Comment 6

9 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/b5ba45e6ac73
RTL hosts don't stay visible when the browser window is resized. r=adw

Comment 7

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5ba45e6ac73
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Reporter)

Updated

9 months ago
Flags: qe-verify+
Backed out on request

Backout: https://hg.mozilla.org/mozilla-central/rev/056a3c3fcc42cf5e22bfa979b0390b3d88d83b65
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

8 months ago
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.
(Assignee)

Comment 10

8 months ago
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 <mbonardo@mozilla.com>
(Assignee)

Comment 12

8 months ago
Hope it's OK if I steal this, Marco.  I had wanted to get a patch while you were on PTO.
Assignee: mak77 → adw
Status: REOPENED → ASSIGNED
(Assignee)

Comment 13

8 months ago
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%.
(Assignee)

Comment 14

8 months ago
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
(Assignee)

Comment 15

8 months ago
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 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.
Attachment #9004742 - Flags: review+

Comment 17

8 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76ccb62dd14d
RTL hosts don't stay visible when the browser window is resized. r=mak

Comment 18

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76ccb62dd14d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago8 months ago
Resolution: --- → FIXED
(Assignee)

Comment 20

8 months ago
I can't get this to fail locally, going to take some time to debug
Status: REOPENED → ASSIGNED
Flags: needinfo?(adw)
(Assignee)

Comment 21

8 months ago
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.
(Assignee)

Comment 22

8 months ago
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

8 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2dcf1b6dd09
RTL hosts don't stay visible when the browser window is resized. r=mak

Comment 24

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2dcf1b6dd09
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Reporter)

Comment 25

8 months ago
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.
Flags: needinfo?(adw)
(Assignee)

Comment 26

8 months ago
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.
Flags: needinfo?(adw)
(Reporter)

Comment 27

8 months ago
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.