Disable ContentChangeObserver on youtube.com
Created attachment 376000 [details] Patch
<rdar://problem/54015403>
Created attachment 376001 [details] Patch
Comment on attachment 376001 [details] Patch We should almost certainly do this only in mobile content mode. Otherwise, this change is likely to break YouTube on iPad as it was necessary for reasonable hover interaction. (ContentChangeObserver isn't a web platform feature, it's just part of the new hover heuristic for desktop websites on iPad.)
Comment on attachment 376001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376001&action=review > Source/WebCore/page/Quirks.cpp:141 > +bool Quirks::shouldDisableContentChangeObserver() const I'm not sure where or how to check if it's in mobile content mode. I have a Document here. Any tips would be appreciated.
I'm going to bed. If someone wants to take this one home, be my guest.
(In reply to Alex Christensen from comment #5) > Comment on attachment 376001 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376001&action=review > > > Source/WebCore/page/Quirks.cpp:141 > > +bool Quirks::shouldDisableContentChangeObserver() const > > I'm not sure where or how to check if it's in mobile content mode. I have a > Document here. Any tips would be appreciated. You want WebsitePoliciesData
(In reply to Tim Horton from comment #7) > (In reply to Alex Christensen from comment #5) > > Comment on attachment 376001 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=376001&action=review > > > > > Source/WebCore/page/Quirks.cpp:141 > > > +bool Quirks::shouldDisableContentChangeObserver() const > > > > I'm not sure where or how to check if it's in mobile content mode. I have a > > Document here. Any tips would be appreciated. > > You want WebsitePoliciesData That doesn't seem to exist anywhere in WebCore, and it also doesn't seem to directly contain the content mode (though it does contain some things controlled by it, like meta viewport mode). In fact I don't think any explicit knowledge of the content mode exists in the WebContent process. Could you be a bit more explicit in your suggestion? Does WebsitePoliciesData contain something that can be used to infer the content mode, and if so how does one get to it from WebCore?
(In reply to Maciej Stachowiak from comment #8) > (In reply to Tim Horton from comment #7) > > (In reply to Alex Christensen from comment #5) > > > Comment on attachment 376001 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=376001&action=review > > > > > > > Source/WebCore/page/Quirks.cpp:141 > > > > +bool Quirks::shouldDisableContentChangeObserver() const > > > > > > I'm not sure where or how to check if it's in mobile content mode. I have a > > > Document here. Any tips would be appreciated. > > > > You want WebsitePoliciesData > > That doesn't seem to exist anywhere in WebCore, and it also doesn't seem to > directly contain the content mode (though it does contain some things > controlled by it, like meta viewport mode). In fact I don't think any > explicit knowledge of the content mode exists in the WebContent process. Agreed, but it's where you would go with such a bit if you were to add one. > Could you be a bit more explicit in your suggestion? Does > WebsitePoliciesData contain something that can be used to infer the content > mode, and if so how does one get to it from WebCore? You add a bit to WebsitePoliciesData, a bit to DocumentLoader, propagate it in WebsitePoliciesData::applyToDocumentLoader, and then you can get to it from WebCore. Not beautiful, but it's how all the others work.
Comment on attachment 376001 [details] Patch Rather than disable all content observation, i think it would be better to figure out which specific behavior causes the bug. Content Observation is about whether to simulate hover or just send simulated clicks. There's a log channel you can turn on to figure out what it's doing.
Good morning! I'm back on this. I'll add a mobile content only check and bisect this change to find out which behavior change is needed.
Created attachment 376013 [details] Patch
Alan is looking into whether the quirk check should be moved to ContentChangeObserver::shouldObserveVisibilityChangeForElement (which I verified also fixes the problem) or an even less invasive change that has lower risk.
Comment on attachment 376013 [details] Patch This change seems to address previous comments and looks like it will work. I'm gonna guess quirks are untestable, but if not I'd suggest a test case. I think it would be good to land some working version ASAP, and consider refining further from there. r=me
the mobile version of youtube.com 1. constructs a visible and actionable content 2. attaches it to the tree 3. and later destroys it completely Content change observer (historically) stops at the first visible change. I've seen this type of behavior on another page and was planning to 1. not stop at the first visible change but instead collect all the qualified nodes. 2. check if the collected nodes are still considered visible right at the end of the observation window.
Alan's plan sounds like a good plan longer term, but too risky for this particular change. I committed the patch to http://trac.webkit.org/r248502
(In reply to Alex Christensen from comment #16) > Alan's plan sounds like a good plan longer term, but too risky for this > particular change. I committed the patch to http://trac.webkit.org/r248502 Agree!