Closed
Bug 1386977
Opened 7 years ago
Closed 7 years ago
No page load detected for pages manipulating the browser history (pushstate/popstate)
Categories
(Remote Protocol :: Marionette, enhancement, P1)
Remote Protocol
Marionette
Tracking
(firefox56 fixed, firefox57 fixed)
RESOLVED
FIXED
mozilla57
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug)
Details
(Whiteboard: [spec][17/08])
Attachments
(1 file)
Originally reported as: https://github.com/mozilla/geckodriver/issues/815
In some cases Marionette doesn't see any page load events and as such waits until the page load times out. Here a test case:
Steps:
1) Open https://www.nrk.no/valg/2017/valgomat/
2) Click one of the smilies
3) Click on "Neste" which will appear as overlay of the image
4) Click the Back button
With step 3 we have a navigation to a new site, but none of the usual page load events we have registered for (beforeunload, pagehide, DOMContentLoaded, pageshow) are caught. As such the click command aborts early.
With step 4 we would also expect those events to fire but there aren't any neither.
Olli, when we talked lately about those events you did some testing. Mind telling me how I can easily see which events are fired in those cases? Is there a section for that in the developer tools? Or how are you doing that? Thanks.
Flags: needinfo?(bugs)
Assignee | ||
Comment 1•7 years ago
|
||
The page uses React (https://facebook.github.io/react/), so maybe the framework is doing something nifty with the location? When I check via the network inspector of devtools, I also do not see a page load including no DOMContentLoaded or pageshow events.
So how does it work when the location changes that no events are fired? Please note that we listen for the events in a frame script, so inside the content process. Interesting is that with the location change we also add a new item to the bfcache.
Comment 2•7 years ago
|
||
How are the event listeners registered? Using which phase and which group?
Flags: needinfo?(bugs)
Comment 3•7 years ago
|
||
(if you're using bubble phase in default group, page can call stopPropagation() and your listeners won't get called.)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2)
> How are the event listeners registered? Using which phase and which group?
It's still the same as we do in bug 1368434 and I'm waiting for a ni? from you. Maybe we can move it here now? So I have no idea what group means, so maybe can shed more details on it?
The registering for relevant events is happening here:
https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/testing/marionette/listener.js#148-200
We do that before we trigger the navigation, so all the listeners are attached.
Flags: needinfo?(bugs)
Comment 5•7 years ago
|
||
So the listeners are added to bubble phase in the default group. Any page calling stopPropagation() would
prevent marionette listeners to get the events.
Flags: needinfo?(bugs)
Assignee | ||
Comment 6•7 years ago
|
||
What I would have to change to make it working? Is it by using mozSystemGroup=true when registering the event listeners, or are some more things necessary?
Flags: needinfo?(bugs)
Comment 7•7 years ago
|
||
Depends on how they should work. If it is ok to to get the listeners called before the web page, then just use capture phase.
If after web page is better, then system group.
Flags: needinfo?(bugs)
Assignee | ||
Comment 8•7 years ago
|
||
Ideally we want to have those listeners called afterward I think. But I tried both to check the behavior but none of them are working for me for this specific website. No events are coming through.
When I use the developer tools and check in the inspector for a possible registered "onbeforeunload", "onpageshow" etc event, I always get back null. So does the page really stop the propagation of events? Is there an easy way to figure that out?
Flags: needinfo?(bugs)
Comment 9•7 years ago
|
||
Hmm, then it is about something else. (Marionette definitely adds listeners wrong way. )
Are you sure a new page is loaded when Neste is clicked? Could the page just use history.pushState() or history.replaceState() ?
Based on devtools I don't see a new document being loaded.
Flags: needinfo?(bugs)
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9)
> Are you sure a new page is loaded when Neste is clicked? Could the page just
> use history.pushState() or history.replaceState() ?
> Based on devtools I don't see a new document being loaded.
Oh, that's actually a good hint! And indeed as I said in comment 1 I also don't see a full page load. I haven't known about the fact that history manipulation can be done this way. Are there some events we could listen for in such cases? Also when pushState() is used, does it mean that when we are going backward/forward in the history no events will be fired too?
Flags: needinfo?(bugs)
Comment 11•7 years ago
|
||
when going back/forward in history, popstate event should fire. But calling pushState doesn't fire events.
I guess you could add a nsIWebProgressListener and get it notified
http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/docshell/base/nsDocShell.cpp#12334
Flags: needinfo?(bugs)
Assignee | ||
Comment 12•7 years ago
|
||
Ok, so that's indeed the case here:
1502203294767 Marionette TRACE 6 -> [0,15,"goBack",{}]
1502203294896 Marionette DEBUG Received DOM event "popstate" for "https://www.nrk.no/valg/2017/valgomat/"
1502203294902 Marionette TRACE 6 <- [1,15,null,{}]
1502203294909 Marionette TRACE 6 -> [0,16,"goForward",{}]
1502203295127 Marionette DEBUG Received DOM event "popstate" for "https://www.nrk.no/valg/2017/valgomat/paastand/1701"
(In reply to Olli Pettay [:smaug] from comment #11)
> when going back/forward in history, popstate event should fire. But calling
> pushState doesn't fire events.
So given the definition on MDN (https://developer.mozilla.org/en-US/docs/Web/API/History_API#The_pushState()_method) we might not have to wait for an event here, given that Firefox isn't going to load the URI at all:
> URL — The new history entry's URL is given by this parameter. Note that the browser
> won't attempt to load this URL after a call to pushState(), but it might attempt to
> load the URL later, for instance after the user restarts the browser.
Which means it should not hurt us when Marionette doesn't wait. Also when the page added with pushState() gets navigated to later from a different URL we get the usual page load events:
1502203788346 Marionette TRACE 6 -> [0,18,"goBack",{}]
1502203788350 Marionette DEBUG Received DOM event "beforeunload" for "http://example.org/"
1502203788570 Marionette DEBUG Received DOM event "pagehide" for "http://example.org/"
1502203788571 Marionette DEBUG Received DOM event "unload" for "http://example.org/"
1502203789059 Marionette DEBUG Received DOM event "DOMContentLoaded" for "https://www.nrk.no/valg/2017/valgomat/paastand/1701"
1502203789321 Marionette DEBUG Received DOM event "pageshow" for "https://www.nrk.no/valg/2017/valgomat/paastand/1701"
1502203789329 Marionette TRACE 6 <- [1,18,null,{}]
1502203789348 Marionette TRACE 6 -> [0,19,"goBack",{}]
1502203789468 Marionette DEBUG Received DOM event "popstate" for "https://www.nrk.no/valg/2017/valgomat/"
1502203789492 Marionette TRACE 6 <- [1,19,null,{}]
Thank you Olli for the help!
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: For some websites no page loads are detected because of missing page load events → No page load detected for pages manipulating the browser history (pushstate/popstate)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895009 -
Flags: review?(ato)
Comment 14•7 years ago
|
||
Are you sure WebDriver should take into account history
manipulation/popstate when navigating?
Assignee | ||
Comment 15•7 years ago
|
||
If we don't take this into account we experience page load timeouts all the time when tests are navigating back and forward. So supporting this is mandatory. Also this is an official HTML5 feature which will be used by a lot of sites.
Also the spec only talks about document ready state. We handle all those steps via page load events. So nothing is in conflict with the spec here. We still return when the page as loading has the correct state as given by the page load strategy.
Assignee | ||
Updated•7 years ago
|
Attachment #8895009 -
Flags: review?(ato) → review?(dburns)
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8895009 [details]
Bug 1386977 - Handle popstate events for page loads.
https://reviewboard.mozilla.org/r/166138/#review171832
::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:225
(Diff revision 1)
> +
> + # By using pushState() the target page is not loaded
> + with self.assertRaises(errors.NoSuchElementException):
> + self.marionette.find_element(By.ID, "target")
> +
> + self.marionette.go_back()
Can you check that the element that shouldnt be there after clicking on forward is there when you go back
Attachment #8895009 -
Flags: review?(dburns) → review+
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895009 [details]
Bug 1386977 - Handle popstate events for page loads.
https://reviewboard.mozilla.org/r/166138/#review171832
> Can you check that the element that shouldnt be there after clicking on forward is there when you go back
When navigating back in history we reach the originally loaded web page which has the link with the id "forward". The element with the id "target" we will first see when navigation_pushstate_target.html is reloaded.
I will add URL checks to make sure we navigate correctly.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895009 [details]
Bug 1386977 - Handle popstate events for page loads.
https://reviewboard.mozilla.org/r/166138/#review171832
> When navigating back in history we reach the originally loaded web page which has the link with the id "forward". The element with the id "target" we will first see when navigation_pushstate_target.html is reloaded.
>
> I will add URL checks to make sure we navigate correctly.
I also added a new test for refresh() with the last update which I missed before.
Comment 20•7 years ago
|
||
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74d0c75abca5
Handle popstate events for page loads. r=automatedtester
Comment 21•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Updated•7 years ago
|
status-firefox56:
--- → fix-optional
Assignee | ||
Comment 22•7 years ago
|
||
Minimal changes with a very low risk of regressions. Would be great to get this test-only change uplifted to beta. Thanks.
Whiteboard: [checkin-needed-beta]
Comment 23•7 years ago
|
||
bugherder uplift |
Assignee | ||
Updated•7 years ago
|
Whiteboard: [spec][17/08]
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•