Closed Bug 1125422 Opened 9 years ago Closed 9 years ago

[AccessFu] Touch and mouse events are not blocked from content

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: eeejay, Assigned: kats)

References

Details

(Whiteboard: [input-thread-uplift-part8])

Attachments

(7 files, 7 obsolete files)

1.34 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.62 KB, text/plain
Details
6.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.00 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.28 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.50 KB, patch
kats
: review+
Details | Diff | Splinter Review
Even if touch and mouse events get preventDefault() on the top window, child mozbrowser frames still get them. This issue was introduced here:
https://hg.mozilla.org/mozilla-central/rev/380b784bf8ec
Additional info from bug 920036:

(In reply to Eitan Isaacson [:eeejay] from comment #40)
> Looks like the last patch on this bug breaks the screen reader.
> We do preventDefault() from the top level window with the intention of
> capturing and redirecting all touch input. Unfortunately, after this patch
> landed, child remote iframes get the events nonetheless. Don't know enough
> about this code to know why this is.
So I debugged this, and the problem seems to be a disparity between how the code added in bug 920036 determines whether an event was prevent-defaulted in the chrome process for APZ purposes:

   event->mFlags.mDefaultPrevented   // [1]

and how the same determination is made for content processes:

  nsIPresShell::gPreventMouseEvents || event.mFlags.mMultipleActionsPrevented  // [2]

A brief IRC discussion with :dvander suggests that this second way is specific to touch-events.

I'm a bit out of my depth here, but the attached patch does seem to fix this bug. It is along the right lines?

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=a736be6898d4#960
[2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=ce1ab99f8af8#2717
Attachment #8554077 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8554077 [details] [diff] [review]
Part 1 - Correctly determine whether a touch event was prevent-defaulted in the chrome process

Review of attachment 8554077 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me, but I really wish we had some documentation on when these flags are set. It seems to me that any time gPreventMouseEvents is set, the return value from DispatchEvent will also be _eConsumeNoDefault, so I would rather check for _eConsumeNoDefault than the magic global var. However that's probably better to do in a follow-up that cleans all this up, and keep this as a low-risk fix for now.
Attachment #8554077 - Flags: feedback?(bugmail.mozilla) → review+
Just built B2G with the patch and it does not seem to fix the issue. I'm still able to activate/scroll content in child iframes of the system app when the screen reader is enabled.
(In reply to Yura Zenevich [:yzen] from comment #5)
> Just built B2G with the patch and it does not seem to fix the issue. I'm
> still able to activate/scroll content in child iframes of the system app
> when the screen reader is enabled.

Confirmed. With this patch, the Homescreen no longer scrolls in response to a one-finger pan, but the Contacts app still does.
(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Yura Zenevich [:yzen] from comment #5)
> > Just built B2G with the patch and it does not seem to fix the issue. I'm
> > still able to activate/scroll content in child iframes of the system app
> > when the screen reader is enabled.
> 
> Confirmed. With this patch, the Homescreen no longer scrolls in response to
> a one-finger pan, but the Contacts app still does.

The problem is that APZ relies on an event hitting the dispatch-to-content (DTC) region of its target layer ([1], targetConfirmed = false) to even wait for Gecko to run touch handlers and possibly prevent-default the event.

Here, the layer containing the element with the touch-listener correctly gets a DTC region, but we still hit-test its descendants, and rely on not finding a better match among them.

However, the descendants are in a different process, and Gecko's "if an ancestor has touch listeners, make my hit-region empty" logic [2] doesn't check ancestors in a different process. As a result, the descendants (the Homescreen's iframe and the Contacts app's iframe) are better matches, and we don't wait for Gecko to run touch handlers.

In the Homescreen case, things happened to work by accident because the Homescreen has a DTC region of its own. (The Contacts app doesn't.)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputQueue.cpp?rev=992050dc1e5f#197
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=301bf68c9566#3127
Discussed this with Kats on IRC. 

We can't really expect Gecko in the child process to know that an element in the parent process has touch-listeners. Instead, we can modify the APZ hit testing algorithm to ignore hit results in a descendant layer in a child process if there is a DTC hit result in an ancestor layer in the parent process.
Assignee: nobody → botond
(In reply to Botond Ballo [:botond] from comment #7)
> Here, the layer containing the element with the touch-listener correctly
> gets a DTC region,

Er, that was conjecture :)

I implemented the fix described in comment 8 (which I expect it still necessary), but we have the additional problem of there not being a DTC region where we want one to be.
The touch-listener in question is being attached to the main chrome window [1].

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js?rev=c636c1f0e07b#361
Attachment #8554077 - Attachment description: Tentative fix → Part 1 - Correctly determine whether a touch event was prevent-defaulted in the chrome process
These patches implement the fix described in comment 8.

Will wait with posting them for review until the remaining problem (described in comment 9) is fixed.
Comment on attachment 8554702 [details] [diff] [review]
Part 2a - Store the layers id in HitTestingTreeNode

Review of attachment 8554702 [details] [diff] [review]:
-----------------------------------------------------------------

The APZC already has a guid and layers id, can't you just use that?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> The APZC already has a guid and layers id, can't you just use that?

I initially thought that, too, but I don't think it will work. Consider the following layer tree:

A     has APZC with DTC
|
--- process boundary
|
B     no APZC
|
C     has APZC

If we detect a process boundary when node->GetNearestApzc()->GetGuid().mLayersId changes, we'll detect the process boundary as being between B and C, rather than between A and B. By that time, we'll have hit the hit-region of B, which is an incorrect hit testing result.
Ah, good point. Carry on, then! :p
Some more discussion with Kats revealed that, in addition to fixing what's been mentioned so far, we're going to need to have APZ enabled in the parent process (bug 950934) for this to work. Otherwise, the layer with the DTC due to the touch listener will not have a containing APZC, and we will discard the event altogether.
Kats and I have a theory that the problem is that we don't pick up touch listeners on the window, because we only pick up listeners on elements with an associated frame, and the window doesn't have one.

Here is our attempt to detect touch listeners on the window. However, it's not working - either we have the wrong window, or the wrong touch listener, or a mistake somewhere else.
Comment on attachment 8554813 [details] [diff] [review]
[WIP] Part 3 - Detect touch listeners on the window

I was able to get this code to detect the touch listener on the chrome window, but I found that setting the mAncestorHasScrollEventHandler flag in PaintFrame doesn't work, because subdocuments clear this flag [1].

Subdocuments clearing the flag is generally correct, because if the event is targerted at the subdocument, touch listeners in ancestor documents don't receive the event and can't prevent-default it - *except* if the listener is chrome, in which get they do get it (and can prevent-default it).

The event-regions building code doesn't currently account for this scenario where there is a chrome handler in an ancestor document.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=07029b5f00e9#487
Attachment #8554813 - Attachment is obsolete: true
Some IRC discussion with :roc and :smaug which may be useful:

[18:57] <roc> so I guess there is some code we need to add to account for the presence of a chrome-event-handler for touch events
[18:58] <botond> roc: yeah, it looks like we need to account for that in the event-regions code
[18:59] <roc> off the top of my head I don't know the best way to check for that
[18:59] <roc> maybe look at what MayHavePaintEventListener does in nsPresContext.cpp
[19:01] <roc> in nsDisplayListBuilder::EnterPresShell, check if the new doc has a chrome event listener and set a flag in PresShellState to indicate that
[19:02] <roc> then check that flag when we decide whether to add to the event region
[19:13] <botond> roc: to check if a document has a chome event listener, is it something like presShell->GetDocument()->GetInnerWindow()->GetChromeEventHandler()->GetExistingListenerManager()->HasListenersFor(...) ?
[19:15] <roc> yeah
[19:21] <smaug> botond: what are you trying to do?
[19:22] <smaug> (use of chromeventhandler that way explictly for any HasListenersFor looks suspicious)
[19:23] <botond> smaug: detect whether, in the context of a subdocument, there is a chrome event handler for touch events
[19:24] <botond> smaug: because then that chrome event handler might prevent-default events otherwise destined toward the subdocument
[19:25] <smaug> well, any ancestor of chrome event handler might have listeners for touch events
[19:25] <smaug> or the parentTarget of GetInnerWindow
[19:25] <botond> smaug: and to account for this, frames in the subdocument need to be added to the dispatch-to-content region of any layers for the subdocument
[19:26] <botond> smaug: what is the parentTarget of GetInnerWindow?
[19:27] <smaug> events go from element->parentNode{0,}->document->window->messagemanager-><xul:browser>(aka chromeEventHandler)->parentNode{0,}->document->window->WindowRoot
[19:27] <smaug> where element is something in non-e10s content page 
[19:29] <smaug> botond: it is possibly enough if you check whether messagemanager and chromeeventhandler->ownerdocument->GetInnerWindow have touch event listeners
[19:29] <smaug> because of http://mxr.mozilla.org/mozilla-central/source/dom/events/EventListenerManager.cpp#332
[19:30] <smaug> so if any element in xul has touch event listeners, the window object in xul has been marked to http://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#441
[19:31] <smaug> (but adding event listeners to messagemanager doesn't have the same effect)
[19:31] <botond> smaug: won't that give me false positives, though?
[19:32] <smaug> possibly yes
[19:32] <smaug> other option is to go through the whole event target chain from messagemanager up and check all the targets there
[19:32] * smaug doesn't know the use cae
[19:32] <smaug> case
[19:33] <smaug> (EventListenerManager does have MayHaveTouchEventListener() for fast check)
[19:34] <botond> smaug: the use case is that if the chrome window has a touch event listener, i would like to know in subdocuments (in the same process)
[19:34] <botond> smaug: a false negative gives incorrect behaviour. a false positive just give slow behaviour (but i'd still like to avoid that)
[19:35] <botond> smaug: (the reason we have slow behaviour is that we can't async-scroll the subdocument if the touch event listener might prevent-default the event)
[19:37] <smaug> hmm, didn't chrome change their behavior to always async scroll anyway or something
[19:37] <smaug> botond: anyhow, this case happens only for certain events, right?
[19:38] <botond> smaug: yes, touch-start and touch-move. this is what we currently check: https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1928
[19:38] <botond> smaug: but that check misses the listener on the chrome window
[19:43] <smaug> botond: hmm, shouldn't we check only the first touchmove... but maybe that code is for something else
[19:44] <botond> smaug: the handler is only allowed to prevent-default the first touch-move event, yes
[19:44] <smaug> botond: this is displaylist building, so super hot code
[19:44] <smaug> thinking
[19:45] <botond> smaug: but there is no way to ask "do you have a listener for a touch-move event that handles the first one" :)
[19:45] <smaug> true
[19:45] <smaug> so we do have code to get the event target chain and store that in an array which one could then iterate 
[19:46] <smaug> basically check targetinarray->MayHaveTouchEventListener() && (targetinarray->HasListenersFor(nsGkAtoms::ontouchstart) || targetinarray->HasListenersFor(nsGkAtoms::ontouchmove))
[19:46] <smaug> but that might not be fast enough
[19:49] <smaug> botond: does this code run basically whenever we ...do hit testing?
[19:50] <roc> smaug: no. this runs during painting, so we can tell the compositor which parts of each layer need sync vs async scrolling.
I think we need to adapt our event-regions code, so that when there's a touch event listener on a <browser> or its ancestor in the event handling chain, we set a flag on the <browser>'s layer (either a RefLayer or a ContainerLayer I guess) to say so. Then, any events going to that layer *or any of its descendants* should be treated as dispatch-to-content.
Alternatively, we could say that chrome touch event handlers don't get to preventDefault subdocument touch events, and we provide an explicit attribute on <browser> that forces dispatch-to-content for the entire layer subtree. I guess that's not much less work than comment #20 though.
Oh, looks like my comment from yesterday didn't get through. It was:

So EventDispatcher::Dispatch can give one the event target chain for aTarget if 
pointer to a nsCOMArray<EventTarget> is passed as the last param.
But that is a bit slow because of all the addref/release event target chain handling needs to do.
A bit faster would be to make that
nsTArray<EventTarget*> and then change the existing callers to copy that array
to nsCOMArray<EventTarget> [1]. But that might not speed it up too much (because event target chain creation anyway
addrefs, meaning that all the objects will be in cycle collector's purple buffer already).

Then 
for (uint32_t i = 0; i < targets.Length(); ++i) {
  EventListenerManager* elm = targets[i]->GetExistingListenerManager();
  if (elm && elm->MayHaveTouchEventListeners() &&
      (elm->HasListenersFor(nsGkAtoms::ontouchstart) || elm->HasListenersFor(nsGkAtoms::ontouchmove))) {
    return true;
  }
}
return false;
That would be correct and not super slow, but I wouldn't be surprised if it showed up in profiles.

This could be optimized by first check
targetElement->GetComposedDoc()->GetInnerWindow()->HasTouchEventListeners() ||
targetElement->GetComposedDoc()->GetInnerWindow()->GetParentTarget()->GetExistingListenerManager()->MayHaveTouchEventListener() ||
targetElement->GetComposedDoc()->GetInnerWindow()->GetChromeEventHandler()->GetComposedDoc()->GetInnerWindow()->HasTouchEventListeners() ||
targetElement->GetComposedDoc()->GetInnerWindow()->->GetChromeEventHandler()->GetComposedDoc()->GetInnerWindow()->GetParentTarget()->GetExistingListenerManager()->MayHaveTouchEventListener()
and only if this expression is true, then use the slow path and check whether there actually is a listener.


[1] Looks like EventListenerService::GetEventTargetChainFor needs to deal with addreffing anyway, so changing
the array from nsCOMArray to nsTArray should be quite trivial.



but we sure could cache the state in <browser>'s layer or something.
Thanks :roc and :smaug for the pointers! I'm going to return to this bug after the parent-process-apz work.
Botond, are you still planning to get back to this now that bug 1005815 is fixed? I am dependent on the screen reader working properly, and it doesn't do that unless this bug is fixed. i can't test nightly builds of FF OS, since I can't open any apps or do much else useful with the device with the screen reader.
Flags: needinfo?(botond)
Botond is away today but either he or I will pick this up today or tomorrow. I think this is the most important outstanding bug we have at the moment. In the meantime I would like to confirm how to enable the screen reader on B2G - what I did was go to the developer settings and enable "show screen reader settings" and then go into the accessibility settings and turn on the screen reader. Is that it, or is there anything else that needs to be done?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> Botond is away today but either he or I will pick this up today or tomorrow.
> I think this is the most important outstanding bug we have at the moment. In
> the meantime I would like to confirm how to enable the screen reader on B2G
> - what I did was go to the developer settings and enable "show screen reader
> settings" and then go into the accessibility settings and turn on the screen
> reader. Is that it, or is there anything else that needs to be done?

Yes that should be enough. You then explore by touch or swipe left and right to sequentially navigate. The activation should then be performed with double tap.
(In reply to Botond Ballo [:botond] from comment #12)
> Created attachment 8554703 [details] [diff] [review]
> Part 2b - During APZ hit-testing, do not descend into child processes if a
> dispatch-to-content region was hit in a parent process
> 
> These patches implement the fix described in comment 8.
> 
> Will wait with posting them for review until the remaining problem
> (described in comment 9) is fixed.

For the record the reason these "part 2" patches don't work even if we have the DTC region set up properly is because the DTC region only shows on PaintedLayers and there may not be a PaintedLayer that gets hit in the parent process. Consider the attached tree dump for example, which is taken from the homescreen with the screen reader enabled (I added l=<layersid> to the dump). All the root-process layers have a DTC region that encompasses their full hit region. However the hit-test still finds the child process' layer because there's nothing on top of it from the root process.

I think roc's suggestion from comment 20 makes sense and would probably address this issue. I'll try to implement it.
Assignee: botond → bugmail.mozilla
Flags: needinfo?(botond)
Attachment #8554702 - Attachment is obsolete: true
Attachment #8554703 - Attachment is obsolete: true
The old part 1 combined with the new parts 2-4 I just attached fix the bug as far as I can tell. Part 2 and 4 I think are fine, but part 3 needs more work still as it only covers a subset of the cases we want and may not be entirely correct. It's correct enough to fix this bug but maybe not correct enough to land.
Is there a reason to prefer adding a flag rather than just making the layer's dispatch-to-content region cover its entire bounds?
(In reply to Botond Ballo [:botond] from comment #32)
> Is there a reason to prefer adding a flag rather than just making the
> layer's dispatch-to-content region cover its entire bounds?

The goal of this suggestion was to avoid adding a special case to APZ hit testing, as the Part 4 patch does, but I realized this would require my original Part 2 patch, which also adds a (different) special case, so there's really no point.
This is mostly so that the list of events we listen for is in a reusable place in nsLayoutUtils.
Attachment #8559917 - Flags: review?(roc)
As per smaug's suggestion in comment 22. I'd flag him for review but his bugzilla status says he's in review overload, so I'll wait until next week.
A few points about this patch:

1) The bits in RenderFrameParent might not be needed, I'm not entirely sure. I think the window that owns that RFP will already have been dealt with either in PaintRoot or nsSubDocumentFrame, and listeners in the child process will be dealt with in its own PaintRoot. It's easy to take out those bits if needed, so I figured I'd leave them in the patch for now (rather than not have it in the patch and then require a re-review when adding them).

2) I considered moving the call to find the listeners into EnterPresShell (and it's easy to do) but I wasn't sure if it was worth it because it's only going to get called once per window anyway.

3) I would like smaug to also review the event listener-checking code but as he's overloaded at the moment I will request review later.
Attachment #8559124 - Attachment is obsolete: true
Attachment #8559923 - Flags: review?(roc)
Attachment #8559125 - Attachment is obsolete: true
Attachment #8559924 - Flags: review?(botond)
eeejay, also if you have some time would you mind applying these patches and verifying that it resolves the accessfu issues? I tested locally but any extra testing would be appreciated.
Flags: needinfo?(eitan)
Comment on attachment 8559924 [details] [diff] [review]
Part 6 - Use the flag in APZ hit-testing

Review of attachment 8559924 [details] [diff] [review]:
-----------------------------------------------------------------

Naming comment: GetForceDispatchToContentRegion() sounds like a function that returns a region, the "force-dispatch-to-content region". I think this method name would read better as ShouldForceDispatchToContentRegion().

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +125,5 @@
>     * because we may use the composition bounds of the layer if the clip is not
>     * present. This value is in L's ParentLayerPixels. */
>    Maybe<nsIntRegion> mClipRegion;
> +
> +  /* If this flag is set, then events to this node and the entire subtree under

s/events to this node/events targeted at this node
Attachment #8559924 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39)
> eeejay, also if you have some time would you mind applying these patches and
> verifying that it resolves the accessfu issues? I tested locally but any
> extra testing would be appreciated.

Did some testing, and this works great.
Flags: needinfo?(eitan)
Comment on attachment 8559923 [details] [diff] [review]
Part 5 - Populate the flag in layout

Review of attachment 8559923 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +1631,5 @@
>        containerParameters.mXScale);
> +  root->SetForceDispatchToContentRegion(
> +      aBuilder->IsBuildingLayerEventRegions()
> +      ? nsLayoutUtils::HasWindowLevelListenersForApzAwareEvents(presShell)
> +      : false);

aBuilder->IsBuildingLayerEventRegions() && nsLayoutUtils::HasWindowLevelListenersForApzAwareEvents(presShell)
	
?: operators with "true" or "false" in one of the branches can always be simplified...

@@ +4064,5 @@
>  {
>    MOZ_COUNT_CTOR(nsDisplaySubDocument);
> +  mForceDispatchToContentRegion = aBuilder->IsBuildingLayerEventRegions()
> +    ? nsLayoutUtils::HasWindowLevelListenersForApzAwareEvents(aFrame->PresContext()->PresShell())
> +    : false;

ditto

::: layout/ipc/RenderFrameParent.cpp
@@ +605,5 @@
> +  , mRemoteFrame(aRemoteFrame)
> +{
> +  mForceDispatchToContentRegion = aBuilder->IsBuildingLayerEventRegions()
> +    ? nsLayoutUtils::HasWindowLevelListenersForApzAwareEvents(aFrame->PresContext()->PresShell())
> +    : false;

ditto
Attachment #8559923 - Flags: review?(roc) → review+
When someone adds or removes a chrome event listener, in principle we should trigger a layer tree update in case this flag changed.

Hmm, I guess that's true for regular APZ-relevant listeners. Do we currently handle that anywhere? If we don't, I guess we should fix that.
Attachment #8559920 - Flags: review?(bugs) → review+
Comment on attachment 8559923 [details] [diff] [review]
Part 5 - Populate the flag in layout

I'm not familiar enough with paint lists for review this.
Did you run this through any profiler?

Do we need to call HasWindowLevelListenersForApzAwareEvents all the time, 
or could we limit it to cases when we're actually dealing with
events which apz is interested in?

Why do we start looking for listeners from window?
Attachment #8559923 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #44)
> I'm not familiar enough with paint lists for review this.

I'm mostly interested in your review on the HasWindowLevelListenersForApzAwareEvents function.

> Did you run this through any profiler?

No. I'll do that now.

> Do we need to call HasWindowLevelListenersForApzAwareEvents all the time, 
> or could we limit it to cases when we're actually dealing with
> events which apz is interested in?

I'm not sure I understand. We need to know about the listeners any time we are building the event regions, and those are the cases I call this function in. The event regions stuff is only enabled on B2G at the moment, because that's the only place we have APZ using it. Eventually we'll turn it on across all platforms.

> Why do we start looking for listeners from window?

The existing code [1] takes care of listeners on elements inside the window (technically anything which generates a frame) but that doesn't take care of the window itself. Note that the part 3 patch on this bug refactors the code at [1].

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=e46260e7857a#1905
Updated part 5 to address roc's review comments (carrying r+), and also to check the listeners on the document as smaug pointed out on IRC. I renamed the function to HasDocumentLevelListenersForApzAwareEvents as a result.
Attachment #8559923 - Attachment is obsolete: true
Attachment #8561493 - Flags: review+
I did some profiling on B2G and the new function doesn't show up on the profiles. I don't think I was getting full symbolication of profiles so that might have been a problem but I saw EventDispatcher::Dispatch showing up in other places (outside of PaintRoot) and so I know at least symbol was available. I'm pretty sure that HasDocumentLevelListenersForApzAwareEvents will have a small perf impact because it's only called when we enter a presshell during a paint, or encounter a cross-process child. It should be pretty infrequent.

I also filed bug 1131128 after my discussion with smaug, to verify/ensure the display:contents case is handled correctly.

Also for the record the other approach that smaug and I were discussing was to not bother with HasDocumentLevelListenersForApzAwareEvents, and instead have CheckForApzAwareEventHandlers check the entire target chain of the content. That would be significantly more expensive, and so we would need some sort of caching to mitigate the perf impact. It's certainly an option but I feel like unless we run into other cases which are hard to handle with the current setup we probably don't need to do that yet.
(In reply to Robert O'Callahan (:roc) (out of office, slow reviews) (Mozilla Corporation) from comment #43)
> When someone adds or removes a chrome event listener, in principle we should
> trigger a layer tree update in case this flag changed.
> 
> Hmm, I guess that's true for regular APZ-relevant listeners. Do we currently
> handle that anywhere? If we don't, I guess we should fix that.

I filed bug 1131188 for this.
So even though we have a potential issue with display:contents (bug 1131128) I'm going to go ahead and land these patches (once the tree reopens) because they improve the situation that we have now and I think the display:contents issue is relatively minor in the grand scheme of things. Further discussion over in bug 1131128.
Whiteboard: [NO_UPLIFT][input-thread-uplift-part8]
Comment on attachment 8554077 [details] [diff] [review]
Part 1 - Correctly determine whether a touch event was prevent-defaulted in the chrome process

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none
Attachment #8554077 - Flags: approval-mozilla-b2g37?
Attachment #8559914 - Flags: approval-mozilla-b2g37?
Attachment #8559917 - Flags: approval-mozilla-b2g37?
Attachment #8559920 - Flags: approval-mozilla-b2g37?
Attachment #8561493 - Flags: approval-mozilla-b2g37?
Attachment #8559924 - Flags: approval-mozilla-b2g37?
Attachment #8554077 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8559914 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8559917 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8559920 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8559924 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8561493 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: