Bug 195256 - [ContentChangeObserver] Content observation should be limited to the current document.
Summary: [ContentChangeObserver] Content observation should be limited to the current ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-03 08:58 PST by zalan
Modified: 2019-03-03 22:05 PST (History)
7 users (show)

See Also:


Attachments
Patch (33.11 KB, patch)
2019-03-03 09:56 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (32.98 KB, patch)
2019-03-03 21:26 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2019-03-03 08:58:03 PST
as opposed to observing all the frames on the page.
Comment 1 Radar WebKit Bug Importer 2019-03-03 08:58:44 PST
<rdar://problem/48544402>
Comment 2 zalan 2019-03-03 09:56:36 PST
Created attachment 363459 [details]
Patch
Comment 3 Daniel Bates 2019-03-03 12:36:29 PST
Comment on attachment 363459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363459&action=review

> Source/WebCore/ChangeLog:8
> +        It limits content observation to the target node's owner document.

To other reviewers: I am not for or against this patch.

Why do we want to do make content change observers document-specific? Interactions with an element can do anything including causing a content change in a subframe (same-origin iframe is easy, cross-origin is still possible via postMessage()). We know Microsoft Word online does the crazy with a subframe that effectively follows the cursor. So there is craziness on the web. Like you needed proof, right :)
Comment 4 zalan 2019-03-03 13:19:11 PST
(In reply to Daniel Bates from comment #3)
> Comment on attachment 363459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363459&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        It limits content observation to the target node's owner document.
> 
> To other reviewers: I am not for or against this patch.
> 
> Why do we want to do make content change observers document-specific?
> Interactions with an element can do anything including causing a content
> change in a subframe (same-origin iframe is easy, cross-origin is still
> possible via postMessage()). We know Microsoft Word online does the crazy
> with a subframe that effectively follows the cursor. So there is craziness
> on the web. Like you needed proof, right :)
:) yeah, that is very true. Do you think if that Microsoft Word use case is a "menu hovering"-like case that we try to cover here? I still yet to see a page where the hover triggers a "bring this menu-pane up in a different frame" type of content change.
Comment 5 Daniel Bates 2019-03-03 14:44:06 PST
Comment on attachment 363459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363459&action=review

>>> Source/WebCore/ChangeLog:8
>>> +        It limits content observation to the target node's owner document.
>> 
>> To other reviewers: I am not for or against this patch.
>> 
>> Why do we want to do make content change observers document-specific? Interactions with an element can do anything including causing a content change in a subframe (same-origin iframe is easy, cross-origin is still possible via postMessage()). We know Microsoft Word online does the crazy with a subframe that effectively follows the cursor. So there is craziness on the web. Like you needed proof, right :)
> 
> :) yeah, that is very true. Do you think if that Microsoft Word use case is a "menu hovering"-like case that we try to cover here? I still yet to see a page where the hover triggers a "bring this menu-pane up in a different frame" type of content change.

I don’t know. I could swear that I recall that Microsoft Word has their own callout bar that they show when you tap or tap and hold, but whether that is in a subframe or would be affected by this patch is something I don’t know off the top of my head. There was a time, a long time ago when everyone was using a a frameset and it was absolutely common to click something and for another frame to change. We flatten frames on iOS, for now, but just pointing it out that it was common for different frames to change. Unless you inspect source it will be hard to tell what technique a site is using. Could use the prostenmez :) approach and just “do it” and see what breaks. I personally prefer to the Feynman algorithm though I’ve been known to gamble with “see what breaks” when I like the odds.
Comment 6 Wenson Hsieh 2019-03-03 14:54:39 PST
Comment on attachment 363459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363459&action=review

>>>> Source/WebCore/ChangeLog:8
>>>> +        It limits content observation to the target node's owner document.
>>> 
>>> To other reviewers: I am not for or against this patch.
>>> 
>>> Why do we want to do make content change observers document-specific? Interactions with an element can do anything including causing a content change in a subframe (same-origin iframe is easy, cross-origin is still possible via postMessage()). We know Microsoft Word online does the crazy with a subframe that effectively follows the cursor. So there is craziness on the web. Like you needed proof, right :)
>> 
>> :) yeah, that is very true. Do you think if that Microsoft Word use case is a "menu hovering"-like case that we try to cover here? I still yet to see a page where the hover triggers a "bring this menu-pane up in a different frame" type of content change.
> 
> I don’t know. I could swear that I recall that Microsoft Word has their own callout bar that they show when you tap or tap and hold, but whether that is in a subframe or would be affected by this patch is something I don’t know off the top of my head. There was a time, a long time ago when everyone was using a a frameset and it was absolutely common to click something and for another frame to change. We flatten frames on iOS, for now, but just pointing it out that it was common for different frames to change. Unless you inspect source it will be hard to tell what technique a site is using. Could use the prostenmez :) approach and just “do it” and see what breaks. I personally prefer to the Feynman algorithm though I’ve been known to gamble with “see what breaks” when I like the odds.

I'm not sure if I'd call it a "callout bar", but it is true that Word shows a contextual menu upon tap/long press; this contextual menu is contained in the same subframe as the one that contains the actual selection ("sdx_ow_iframe").
Comment 7 zalan 2019-03-03 18:40:09 PST
(In reply to Daniel Bates from comment #5)
> Comment on attachment 363459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363459&action=review
> 
> >>> Source/WebCore/ChangeLog:8
> >>> +        It limits content observation to the target node's owner document.
> >> 
> >> To other reviewers: I am not for or against this patch.
> >> 
> >> Why do we want to do make content change observers document-specific? Interactions with an element can do anything including causing a content change in a subframe (same-origin iframe is easy, cross-origin is still possible via postMessage()). We know Microsoft Word online does the crazy with a subframe that effectively follows the cursor. So there is craziness on the web. Like you needed proof, right :)
> > 
> > :) yeah, that is very true. Do you think if that Microsoft Word use case is a "menu hovering"-like case that we try to cover here? I still yet to see a page where the hover triggers a "bring this menu-pane up in a different frame" type of content change.
> 
> I don’t know. I could swear that I recall that Microsoft Word has their own
> callout bar that they show when you tap or tap and hold, but whether that is
> in a subframe or would be affected by this patch is something I don’t know
> off the top of my head. There was a time, a long time ago when everyone was
> using a a frameset and it was absolutely common to click something and for
> another frame to change.
Right and that's fine since the change in triggered by a click (and not by mouse-move). I've seen many pages like that, but again not when the content change is triggered by only hovering over an element.

>We flatten frames on iOS, for now, but just pointing it out that it was common for different frames to change. 
Frame flattening is strictly a rendering feature only and does not alter the frame (document) hierarchy. From content change observation point of view, it does not matter whether the a frame is flattened or not.
Comment 8 Simon Fraser (smfr) 2019-03-03 19:31:44 PST
Comment on attachment 363459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363459&action=review

>>>>>> Source/WebCore/ChangeLog:8
>>>>>> +        It limits content observation to the target node's owner document.
>>>>> 
>>>>> To other reviewers: I am not for or against this patch.
>>>>> 
>>>>> Why do we want to do make content change observers document-specific? Interactions with an element can do anything including causing a content change in a subframe (same-origin iframe is easy, cross-origin is still possible via postMessage()). We know Microsoft Word online does the crazy with a subframe that effectively follows the cursor. So there is craziness on the web. Like you needed proof, right :)
>>>> 
>>>> :) yeah, that is very true. Do you think if that Microsoft Word use case is a "menu hovering"-like case that we try to cover here? I still yet to see a page where the hover triggers a "bring this menu-pane up in a different frame" type of content change.
>>> 
>>> I don’t know. I could swear that I recall that Microsoft Word has their own callout bar that they show when you tap or tap and hold, but whether that is in a subframe or would be affected by this patch is something I don’t know off the top of my head. There was a time, a long time ago when everyone was using a a frameset and it was absolutely common to click something and for another frame to change. We flatten frames on iOS, for now, but just pointing it out that it was common for different frames to change. Unless you inspect source it will be hard to tell what technique a site is using. Could use the prostenmez :) approach and just “do it” and see what breaks. I personally prefer to the Feynman algorithm though I’ve been known to gamble with “see what breaks” when I like the odds.
>> 
>> I'm not sure if I'd call it a "callout bar", but it is true that Word shows a contextual menu upon tap/long press; this contextual menu is contained in the same subframe as the one that contains the actual selection ("sdx_ow_iframe").
> 
> Right and that's fine since the change in triggered by a click (and not by mouse-move). I've seen many pages like that, but again not when the content change is triggered by only hovering over an element.

Googling for "prostenmez" has no hits.

> Source/WebCore/dom/Document.cpp:539
> +    , m_contentChangeObserver(std::make_unique<ContentChangeObserver>(*this))

Maybe make one the first time we need to? Many documents are probably never interacted with.

> Source/WebCore/page/ios/ContentChangeObserver.cpp:46
> -    if (!m_page.mainFrame().document())
> -        return;
> -    if (m_page.mainFrame().document()->activeDOMObjectsAreSuspended())
> +    if (m_document.activeDOMObjectsAreSuspended())

This is so much nicer.
Comment 9 zalan 2019-03-03 21:26:43 PST
Created attachment 363483 [details]
Patch
Comment 10 WebKit Commit Bot 2019-03-03 22:05:50 PST
Comment on attachment 363483 [details]
Patch

Clearing flags on attachment: 363483

Committed r242340: <https://trac.webkit.org/changeset/242340>
Comment 11 WebKit Commit Bot 2019-03-03 22:05:52 PST
All reviewed patches have been landed.  Closing bug.