Closed
Bug 1340592
Opened 7 years ago
Closed 7 years ago
pageshow event persisted=false when page has img with srcset
Categories
(Core :: DOM: Events, defect, P3)
Tracking
()
RESOLVED
INVALID
People
(Reporter: teun, Assigned: edgar)
References
Details
(Keywords: regression)
Attachments
(3 files)
|
845 bytes,
text/html
|
Details | |
|
2.12 KB,
patch
|
Details | Diff | Splinter Review | |
|
6.70 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.2743.82 Safari/537.36 OPR/39.0.2256.43 Steps to reproduce: 1. Put the attached html file pageshow-persisted-srcset.html somewhere on a webserver 2. Visit that page with Firefox 3. Open Firebug (and enable the console) 4. Refresh the page (deeply). 5. Observe that evt.persisted is false 6. Visit the link to google.com 7. Press the back button in the browser 8. Observe that evt.persisted is still false Actual results: evt.persisted is false Expected results: evt.persisted should be true
Not sure of the regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a6b6a93eb41a05e310a11f0172f01ba9b21d3eac&tochange=541c9086c0f27fba60beecc9bc94543103895c86 When testing STR, don't resize the viewport. alice, could you confirm?
Component: Untriaged → DOM: Events
Flags: needinfo?(alice0775)
Keywords: regression
Product: Firefox → Core
|
||
Comment 2•7 years ago
|
||
(In reply to Loic from comment #1) > Not sure of the regression range: > https://hg.mozilla.org/integration/fx-team/ > pushloghtml?fromchange=a6b6a93eb41a05e310a11f0172f01ba9b21d3eac&tochange=541c > 9086c0f27fba60beecc9bc94543103895c86 > > When testing STR, don't resize the viewport. > > alice, could you confirm? I got a different range(I did not use Firebug, but use Browser Console): https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=40c1e96693fb0efc7850c3f8e550bf552982d191&tochange=1c1cde95bfa6fbec4e6bc3c732ab62709bf2dd95 Suspect: 1c1cde95bfa6 Michael Layzell — Bug 1166138 - Make img srcset react to resize/viewport changes, r=jdm
Flags: needinfo?(alice0775)
|
|
||
Comment 3•7 years ago
|
||
Well, When I open "about:home" instead of "https//www.google.co.jp" at step6, I got same regression window of Comment 1. So, There seems to be at least two problems.
|
||
Comment 4•7 years ago
|
||
Chrome, Safari, and Edge all return false. Are we sure the document is "salvageable" (as per HTML)?
|
||
Comment 5•7 years ago
|
||
Chrome and Safari have the same behaviour, i.e. evt.persisted is false. Hi Edgar, could you please help confirm what the expected spec behaviour is?
Flags: needinfo?(echen)
|
||
Comment 6•7 years ago
|
||
FWIW, Chrome doesn't have bfcache, so .persisted should be always false there. Safari should have bfcache, at least on some platforms.
|
Assignee | |
Comment 7•7 years ago
|
||
SetOverrideDPPX() call [1] in nsDocShell::RestoreFromHistory triggers HTMLImageElement::MediaFeatureValuesChanged [2]. Then HTMLImageElement blocks document onload and runs img source selection steps. And since the viewport doesn't change, so we end up select same source and won't do any image load. But the block-doucument-onload things seems make persisted becomes false. [1] http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/docshell/base/nsDocShell.cpp#8845 [2] http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/dom/html/HTMLImageElement.cpp#1329
Flags: needinfo?(echen)
|
|
Assignee | |
Updated•7 years ago
|
Flags: needinfo?(echen)
|
|
Assignee | |
Comment 8•7 years ago
|
||
Flags: needinfo?(echen)
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → echen
|
|
Assignee | |
Comment 9•7 years ago
|
||
|
|
Assignee | |
Comment 10•7 years ago
|
||
Comment on attachment 8847475 [details] [diff] [review] Test, v1 Review of attachment 8847475 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/chrome/bug1340592_window.xul @@ +102,5 @@ > + > + var test2Doc = "data:text/html,<html><head><title>test2</title></head>" + > + "<body>test2</body></html>"; > + > + gExpected = [{type: "pagehide", title: "test1", persisted: true}, I wrote this test case to verify the fix, it is supposed to work (I have verified the fix manually, and the fix works good). But I ran into a separated issue, and this test got failed due to the page (bug1340592.html) doesn't be saved into bfcache at all. After digging into the code a bit: At the time we call loadURI() (after received "load" event), loadGroup still have pending request and nsDocument::CanSavePresentation returns false . The pending request is from image::ProgressTracker::SyncNotifyProgress(). I believe this is because imageLib sends an extra block/unblock onload pair in bug 1120149 :(
|
||
Comment 11•7 years ago
|
||
+1 on fixing that, we just hit that from devtools codebase when we check for the persisted attribute.
Another test case if that can help:
* open:
data:text/html,<!doctype html><html><img src="foo.png" srcset="foo.png 1.5x"><script>onpagehide=e=>document.body.innerHTML+=("pagehide:"+e.persisted+"<br/>"); onpageshow=e=>document.body.innerHTML+=("pageshow:"+e.persisted+"<br/>");</script></html>
* go to wikipedia.org
* go back
And you will see the misleading persisted=false on pageshow, whereas it was true during pagehide.
Until this is fixed, we will have to workaround that...|
|
||
Comment 12•7 years ago
|
||
FWIW, in general nothing guarantees that if pagehide has persisted==true that going back to that page has pageshow's persisted==true too.
|
|
||
Updated•7 years ago
|
Priority: -- → P3
|
|
Assignee | |
Comment 13•7 years ago
|
||
bug 1379762 seems to also fix this. The test cases in comment 0 and comment 11 cannot be reproduced in current nightly, so close this bug. Feel free to reopen it if you still encounter this issue.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•