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

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/15238710>

[Simon Fraser (smfr)]

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?

[Ryosuke Niwa]

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

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]

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

[Ryosuke Niwa]

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

[Antonio Gomes]

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?

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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

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.

[Ryosuke Niwa]

Created attachment 215460 [details]
Rollout r154614 and r156605

[Ryosuke Niwa]

Created attachment 215462 [details]
Revert the change to HTMLBodyElement

[Oliver Hunt]

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

[Simon Fraser (smfr)]

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

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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]

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

[Ryosuke Niwa]

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?

[Antonio Gomes]

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

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

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

Yup. If I pick Firefox as the UA on Develop menu on Safari, the regression doesn't reproduce.

We're screwed :(

[Antonio Gomes]

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

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

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

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

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]

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

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

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]

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

[Ryosuke Niwa]

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

[Antonio Gomes]

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