Bug 122882

Summary:

REGRESSION(r154614): Opening and closing a picture on Facebook resets scroll position

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Layout and Rendering

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, cmarcelo, commit-queue, darin, dbates, ddkilzer, esprehn+autocc, gur.trio, gyuyoung.kim, jeffrey+webkit, kangil.han, mitz, oliver, simon.fraser, thorton, tonikitoo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

106133

Bug Blocks:

Attachments:

Description	Flags
Rollout r154614 and r156605	none
Revert the change to HTMLBodyElement	none

Ryosuke Niwa

Reported 2013-10-15 21:50:23 PDT

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.

Attachments
Rollout r154614 and r156605 (26.73 KB, patch) 2013-10-29 18:58 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Revert the change to HTMLBodyElement (7.89 KB, patch) 2013-10-29 19:42 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2013-10-15 21:51:11 PDT

<rdar://problem/15238710>

Simon Fraser (smfr)

Comment 2 2013-10-15 22:34:34 PDT

Should we just revert r154614?

gur.trio

Comment 3 2013-10-15 23:44:54 PDT

(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?

Ryosuke Niwa

Comment 4 2013-10-15 23:56:41 PDT

(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.

Ryosuke Niwa

Comment 5 2013-10-16 00:00:36 PDT

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.

Ryosuke Niwa

Comment 6 2013-10-16 00:02:04 PDT

Also see https://bugs.webkit.org/show_bug.cgi?id=121876

gur.trio

Comment 7 2013-10-16 00:04:34 PDT

(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

Ryosuke Niwa

Comment 8 2013-10-16 00:18:41 PDT

(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?

gur.trio

Comment 9 2013-10-16 00:26:52 PDT

(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.

Antonio Gomes

Comment 10 2013-10-16 00:39:19 PDT

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.

gur.trio

Comment 11 2013-10-16 02:05:27 PDT

(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?

Ryosuke Niwa

Comment 12 2013-10-29 18:48:18 PDT

I'm rolling out https://trac.webkit.org/r154614.

gur.trio

Comment 13 2013-10-29 18:51:39 PDT

(In reply to comment #12) > I'm rolling out https://trac.webkit.org/r154614. Is the fix wrong?

Ryosuke Niwa

Comment 14 2013-10-29 18:52:50 PDT

(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.

Ryosuke Niwa

Comment 15 2013-10-29 18:53:37 PDT

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.

gur.trio

Comment 16 2013-10-29 18:56:47 PDT

(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.

Ryosuke Niwa

Comment 17 2013-10-29 18:58:23 PDT

Created attachment 215460 [details] Rollout r154614 and r156605

Ryosuke Niwa

Comment 18 2013-10-29 19:42:24 PDT

Created attachment 215462 [details] Revert the change to HTMLBodyElement

Oliver Hunt

Comment 19 2013-10-29 22:15:12 PDT

(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 :-/

gur.trio

Comment 20 2013-10-29 22:23:01 PDT

(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.

Simon Fraser (smfr)

Comment 21 2013-10-29 22:25:38 PDT

(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.

WebKit Commit Bot

Comment 22 2013-10-29 22:37:51 PDT

Comment on attachment 215462 [details] Revert the change to HTMLBodyElement Clearing flags on attachment: 215462 Committed r158254: <http://trac.webkit.org/changeset/158254>

WebKit Commit Bot

Comment 23 2013-10-29 22:37:56 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 24 2013-10-29 22:40:13 PDT

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 :(

Ryosuke Niwa

Comment 25 2013-10-29 22:42:06 PDT

At least we know where to look should we find more regressions caused by the original patch.

Ryosuke Niwa

Comment 26 2013-10-29 22:43:53 PDT

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 :(

gur.trio

Comment 27 2013-10-29 22:45:05 PDT

(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?

Antonio Gomes

Comment 28 2013-10-29 22:46:57 PDT

(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.

Ryosuke Niwa

Comment 29 2013-10-29 22:47:23 PDT

(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.

Ryosuke Niwa

Comment 30 2013-10-29 22:50:11 PDT

(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.

Ryosuke Niwa

Comment 31 2013-10-29 22:53:09 PDT

Yup. If I pick Firefox as the UA on Develop menu on Safari, the regression doesn't reproduce. We're screwed :(

Antonio Gomes

Comment 32 2013-10-29 22:54:37 PDT

(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) :/

Ryosuke Niwa

Comment 33 2013-10-29 23:00:06 PDT

(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.

Oliver Hunt

Comment 34 2013-10-29 23:13:05 PDT

(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?

Ryosuke Niwa

Comment 35 2013-10-29 23:39:37 PDT

(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.

Ryosuke Niwa

Comment 36 2013-10-29 23:40:19 PDT

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.

Antonio Gomes

Comment 37 2013-10-30 06:51:43 PDT

(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.

Ryosuke Niwa

Comment 38 2013-10-30 11:01:22 PDT

(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…

Ryosuke Niwa

Comment 39 2013-10-30 11:34:18 PDT

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.

Ryosuke Niwa

Comment 40 2013-10-30 11:44:45 PDT

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

gur.trio

Comment 41 2013-11-14 23:11:38 PST

(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

Ryosuke Niwa

Comment 42 2013-12-18 18:55:08 PST

(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.

mitz

Comment 43 2014-02-15 12:32:35 PST

(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.

Antonio Gomes

Comment 44 2014-03-03 13:23:27 PST

(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.

Note You need to log in before you can comment on or make changes to this bug.