Bug 195256 – [ContentChangeObserver] Content observation should be limited to the current document.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

as opposed to observing all the frames on the page.

[Radar WebKit Bug Importer]

<rdar://problem/48544402>

[zalan]

Created attachment 363459 [details]
Patch

[Daniel Bates]

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

[zalan]

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

[Daniel Bates]

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.

[Wenson Hsieh]

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").

[zalan]

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

[Simon Fraser (smfr)]

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.

[zalan]

Created attachment 363483 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363483 [details]
Patch

Clearing flags on attachment: 363483

Committed r242340: <https://trac.webkit.org/changeset/242340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.