Reproduction steps 1. Go to Facebook.com (log in) 2. Scroll down and open a picture 3. Close the picture by pressing the escape key Expected result: The picture closes and I'm back to where I was. Actual result: The scroll position is reset so I'm back to the top of the page.
<rdar://problem/15238710>
Should we just revert r154614?
(In reply to comment #2) > Should we just revert r154614? Then how does this issue work fine on mozilla and other browsers where document.documentElement.scrollTop/scrollLeft document.body.scrollTop/scrollLeft is as per the spec. Those browser get a different content based on UA?
(In reply to comment #3) > (In reply to comment #2) > > Should we just revert r154614? > > Then how does this issue work fine on mozilla and other browsers where document.documentElement.scrollTop/scrollLeft document.body.scrollTop/scrollLeft is as per the spec. > > Those browser get a different content based on UA? I've tried both IE and FF UA strings (via Develop menu on Safari) but that didn't help. That seems to indicate that either your patch didn't implement the spec correctly or that the spec is wrong; doesn't reflect what other browsers do.
To begin with, the behavior of which specification did you implement in r154614? I skimmed through the bug and the patch but I couldn't find any name or URL of the specification you're referring to.
Also see https://bugs.webkit.org/show_bug.cgi?id=121876
(In reply to comment #6) > Also see https://bugs.webkit.org/show_bug.cgi?id=121876 http://dev.w3.org/csswg/cssom-view/#dom-element-scrolltop
(In reply to comment #7) > (In reply to comment #6) > > Also see https://bugs.webkit.org/show_bug.cgi?id=121876 > > http://dev.w3.org/csswg/cssom-view/#dom-element-scrolltop Did you check if FF and IE do what's stated in the spec?
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Also see https://bugs.webkit.org/show_bug.cgi?id=121876 > > > > http://dev.w3.org/csswg/cssom-view/#dom-element-scrolltop > > Did you check if FF and IE do what's stated in the spec? yes FF I checked. Based on that and spec I started with the changes. Tested test case like the ones in Layout test cases.
Note that i am doing this work on blink and it was preferable to take a more conservative approach. An use count was added to body.scrolltop/left as well as a console warning about a future deprecation.
(In reply to comment #10) > Note that i am doing this work on blink and it was preferable to take a more conservative approach. An use count was added to body.scrolltop/left as well as a console warning about a future deprecation. Ryosuke anything wrong with the changes as per the spec?
I'm rolling out https://trac.webkit.org/r154614.
(In reply to comment #12) > I'm rolling out https://trac.webkit.org/r154614. Is the fix wrong?
(In reply to comment #13) > (In reply to comment #12) > > I'm rolling out https://trac.webkit.org/r154614. > > Is the fix wrong? Yes. It caused the said regression on Facebook and other websites.
It really doesn't matter how faithfully you implemented the spec. If it causes a major backward compatibility with the Web, we can't have it.
(In reply to comment #15) > It really doesn't matter how faithfully you implemented the spec. If it causes a major backward compatibility with the Web, we can't have it. So then we submit some patch/fix and that is as per spec but breaks sites we will not consider that. If that is the process will keep that in mind. What else can I say. You guys are in a better position to decide. Need to roll back http://trac.webkit.org/changeset/156605 also then.
Created attachment 215460 [details] Rollout r154614 and r156605
Created attachment 215462 [details] Revert the change to HTMLBodyElement
(In reply to comment #16) > (In reply to comment #15) > > It really doesn't matter how faithfully you implemented the spec. If it causes a major backward compatibility with the Web, we can't have it. > > So then we submit some patch/fix and that is as per spec but breaks sites we will not consider that. If that is the process will keep that in mind. What else can I say. You guys are in a better position to decide. > > Need to roll back http://trac.webkit.org/changeset/156605 also then. if matching the spec breaks websites it means that the bug is in the spec :-/
(In reply to comment #19) > (In reply to comment #16) > > (In reply to comment #15) > > > It really doesn't matter how faithfully you implemented the spec. If it causes a major backward compatibility with the Web, we can't have it. > > > > So then we submit some patch/fix and that is as per spec but breaks sites we will not consider that. If that is the process will keep that in mind. What else can I say. You guys are in a better position to decide. > > > > Need to roll back http://trac.webkit.org/changeset/156605 also then. > > if matching the spec breaks websites it means that the bug is in the spec :-/ Then facebook issues and other wesbites break issue should also be reproducible on firefox and other browers which are following the spec. That I am not able to understand.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #16) > > > (In reply to comment #15) > > > > It really doesn't matter how faithfully you implemented the spec. If it causes a major backward compatibility with the Web, we can't have it. > > > > > > So then we submit some patch/fix and that is as per spec but breaks sites we will not consider that. If that is the process will keep that in mind. What else can I say. You guys are in a better position to decide. > > > > > > Need to roll back http://trac.webkit.org/changeset/156605 also then. > > > > if matching the spec breaks websites it means that the bug is in the spec :-/ > > Then facebook issues and other wesbites break issue should also be reproducible on firefox and other browers which are following the spec. > > That I am not able to understand. That suggests the implementation had a bug (or WebKit has some other bug). You should try debugging the Facebook issue after putting the patch back in.
Comment on attachment 215462 [details] Revert the change to HTMLBodyElement Clearing flags on attachment: 215462 Committed r158254: <http://trac.webkit.org/changeset/158254>
All reviewed patches have been landed. Closing bug.
I just found document.body.scrollLeft + document.documentElement.scrollLeft and document.body.scrollTop + document.documentElement.scrollTop on Facebook.com. So the fix that just got landed doesn't work :(
At least we know where to look should we find more regressions caused by the original patch.
17,527 results match for "document.body.scrollLeft + document.documentElement.scrollLeft" on github: https://github.com/search?q=%22document.body.scrollLeft+%2B+document.documentElement.scrollLeft%22&type=Code&ref=searchresults :(
(In reply to comment #24) > I just found > document.body.scrollLeft + document.documentElement.scrollLeft > and > document.body.scrollTop + document.documentElement.scrollTop > on Facebook.com. > > So the fix that just got landed doesn't work :( I didnot get this? Even after rolling back issue is still there on facebook?
(In reply to comment #24) > I just found > document.body.scrollLeft + document.documentElement.scrollLeft > and > document.body.scrollTop + document.documentElement.scrollTop > on Facebook.com. > > So the fix that just got landed doesn't work :( So the original patch should work .. Unless facebook is doing browser detection instead of feature detection. Will try to see facebook's JS too.
(In reply to comment #27) > (In reply to comment #24) > > I just found > > document.body.scrollLeft + document.documentElement.scrollLeft > > and > > document.body.scrollTop + document.documentElement.scrollTop > > on Facebook.com. > > > > So the fix that just got landed doesn't work :( > > I didnot get this? Even after rolling back issue is still there on facebook? The patch that just got landed doesn't fully revert your change. It only reverted the change in HTMLBodyElement. The problem here is that there might be enough Web content that depends on the fact only either property returns non-zero value.
(In reply to comment #28) > So the original patch should work .. Unless facebook is doing browser detection instead of feature detection. > > Will try to see facebook's JS too. Well, they probably have BOTH.
Yup. If I pick Firefox as the UA on Develop menu on Safari, the regression doesn't reproduce. We're screwed :(
(In reply to comment #30) > (In reply to comment #28) > > So the original patch should work .. Unless facebook is doing browser detection instead of feature detection. > > > > Will try to see facebook's JS too. > > Well, they probably have BOTH. That is a big change indeed, and not easy to execute with heavy evangelization. All "documentElement.scrollTop + body.scrollTop" occurrences on GitHub were working property before the patches, after the patches, but not now, with a parcial roll out. Maybe Facebook would work too if Safari's UA was changed to match say Firefox (for exercise proposes) :/
(In reply to comment #32) > > Maybe Facebook would work too if Safari's UA was changed to match say Firefox (for exercise proposes) :/ Yes, the regression doesn't reproduce if we fake the UA string as I mentioned in the comment #31.
(In reply to comment #33) > (In reply to comment #32) > > > > Maybe Facebook would work too if Safari's UA was changed to match say Firefox (for exercise proposes) :/ > > Yes, the regression doesn't reproduce if we fake the UA string as I mentioned in the comment #31. Is the broken code path hidden behind their generic firefox flag, or is it a one off?
(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #32) > > > > > > Maybe Facebook would work too if Safari's UA was changed to match say Firefox (for exercise proposes) :/ > > > > Yes, the regression doesn't reproduce if we fake the UA string as I mentioned in the comment #31. > > Is the broken code path hidden behind their generic firefox flag, or is it a one off? That, I don't know.
But given our own build.webkit.org is broken, I bet there is a lot of mobile web content out there that depends on document.body.scrollTop/scrollLeft.
(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > (In reply to comment #32) > > > > > > > > Maybe Facebook would work too if Safari's UA was changed to match say Firefox (for exercise proposes) :/ > > > > > > Yes, the regression doesn't reproduce if we fake the UA string as I mentioned in the comment #31. > > > > Is the broken code path hidden behind their generic firefox flag, or is it a one off? > > That, I don't know. I believe this shows a Web site, doing browser detection, and work around a WebKit quirk. @rniwa, interesting enough, your bug description works fine with Blink ToT.
(In reply to comment #37) > > I believe this shows a Web site, doing browser detection, and work around a WebKit quirk. > > @rniwa, interesting enough, your bug description works fine with Blink ToT. Yes. What I did was searching through JS code that got loaded into inspector on Facebook so I don't even know where that sum is used :( There is something broken on Facebook somewhere…
So I think my friends at Facebook are going to remove the summation from their code. Hooray! We'll see how pervasive this summation trick is.
A popular JS library, Leafleat uses this summation: https://github.com/Leaflet/Leaflet/blob/1a3b150a2d140f46ae3578e770b607d9ec8c72eb/src/dom/DomEvent.js x = e.pageX ? e.pageX - body.scrollLeft - docEl.scrollLeft: e.clientX y = e.pageY ? e.pageY - body.scrollTop - docEl.scrollTop: e.clientY
(In reply to comment #40) > A popular JS library, Leafleat uses this summation: > https://github.com/Leaflet/Leaflet/blob/1a3b150a2d140f46ae3578e770b607d9ec8c72eb/src/dom/DomEvent.js > > x = e.pageX ? e.pageX - body.scrollLeft - docEl.scrollLeft: e.clientX > y = e.pageY ? e.pageY - body.scrollTop - docEl.scrollTop: e.clientY What is the final conclusion? As I can see in the latest code. HTMLBodyElement.cpp has been reverted but not Element.cpp
(In reply to comment #41) > (In reply to comment #40) > > A popular JS library, Leafleat uses this summation: > > https://github.com/Leaflet/Leaflet/blob/1a3b150a2d140f46ae3578e770b607d9ec8c72eb/src/dom/DomEvent.js > > > > x = e.pageX ? e.pageX - body.scrollLeft - docEl.scrollLeft: e.clientX > > y = e.pageY ? e.pageY - body.scrollTop - docEl.scrollTop: e.clientY > > What is the final conclusion? As I can see in the latest code. HTMLBodyElement.cpp has been reverted but not Element.cpp The conclusion is to wait & see if returning non-zero value on BOTH HTMLBodyElement and Document sticks or not.
(In reply to comment #22) > (From update of attachment 215462 [details]) > Clearing flags on attachment: 215462 > > Committed r158254: <http://trac.webkit.org/changeset/158254> This caused bug 128873.
(In reply to comment #26) > 17,527 results match for "document.body.scrollLeft + document.documentElement.scrollLeft" on github: > https://github.com/search?q=%22document.body.scrollLeft+%2B+document.documentElement.scrollLeft%22&type=Code&ref=searchresults > > :( happily, the Web is moving forward, and the same query returns "only" 503 entries, being lots of these repeated.