1272491 - [e10s] With e10s enabled, hide and show events are not coming through.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

Need to add xpcom parts to Recv[Hide/Show]Event.

A good indicator of this issue is a test that can be found here:
accessible/tests/browser/browser_events_hide.js
accessible/tests/browser/browser_events_show.js

The test will pass with --disable-e10s.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1272491 patch

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9109987482ac

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ad47b206e5

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1272491 patch

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8752287 [details] [diff] [review]
1272491 patch

># HG changeset patch
># User Yura Zenevich <[e-mail]>
>
>Bug 1272491 - adding xpcom parts to show and hide event handling with e10s. r=tbsaunde

fix the english, fire xpcom show / hide events or something.

>@@ -136,16 +150,31 @@ DocAccessibleParent::RecvHideEvent(const uint64_t& aRootID,
>   ProxyAccessible* root = rootEntry->mProxy;
>   if (!root) {
>     NS_ERROR("invalid root being removed!");
>     return true;
>   }
>
>   ProxyAccessible* parent = root->Parent();
>   ProxyShowHideEvent(root, parent, false, aFromUser);
>+
>+  if (nsCoreUtils::AccEventObserversExist()) {
>+    uint32_t type = nsIAccessibleEvent::EVENT_HIDE;
>+    xpcAccessibleGeneric* xpcAcc = GetXPCAccessible(root);
>+    xpcAccessibleGeneric* xpcParent = GetXPCAccessible(parent);
>+    xpcAccessibleGeneric* xpcNext = GetXPCAccessible(root->NextSibling());
>+    xpcAccessibleGeneric* xpcPrev = GetXPCAccessible(root->PrevSibling());
>+    xpcAccessibleDocument* doc = GetAccService()->GetXPCDocument(this);
>+    nsIDOMNode* node = nullptr;
>+    RefPtr<xpcAccHideEvent> event =
>+      new xpcAccHideEvent(type, xpcAcc, doc, node, aFromUser, xpcParent,
>+                          xpcNext, xpcPrev);
>+    nsCoreUtils::DispatchAccEvent(Move(event));

I'd really rather you actually fired the event after proxy->Shutdown(), but if its here any js that runs as an observer *must* not trigger any sync ipc to the child, and async ipc might not be ok either, but I'm not sure about that.

> xpcAccessibleGeneric*
> DocAccessibleParent::GetXPCAccessible(ProxyAccessible* aProxy)
> {
>+  if (!aProxy) {
>+    return nullptr;
>+  }

It seems better to check the two callers where it can actually be null.

>     <div id="previous"></div>
>     <div id="div"></div>
>     <div id="next"></div>
>-  </div>`, function*(browser) {
>-  let onHide = waitForEvent(EVENT_HIDE, 'div');
>+  </div>`, function*(browser, accDoc) {

hiding the function declaration like that is confusing.

>+  let acc = findAccessibleChildByID(accDoc, 'div');

using div as an id is at least confusing, and could be a foot gun.

>+  let onHide = waitForEvent(EVENT_HIDE, acc);

its not really an observer / callback, so that's a odd name, but I don't know that I have a better one.

>
>   /* ================= Remove HTML from iframe document ===================== */

this comment doesn't seem that useful, and its dupplicated below.

>-  onReorder = waitForEvent(EVENT_REORDER);
>+  onReorder = waitForEvent(EVENT_REORDER, iframe);

naming

>  * A helper function that waits for an accessible event of certain type that

of the given type seems like better english

>+ * belongs to a certain DOMNode (defined by its id or accessible).

with the given target

>+function waitForEvent(eventType, expectedIdOrAcc) {
>+  let isID = typeof expectedIdOrAcc === 'string';

move this to where its used, or just eliminate it entirely?

>   return new Promise(resolve => {
>     let eventObserver = {
>       observe(subject, topic, data) {

adding and removing observers all the time seems silly, but its a test so whatever.

>-        let domID = getAccessibleDOMNodeID(event.accessible);
>-        // If event's accessible does not match expected event type or DOMNode
>-        // id, skip thie event.
>-        if (domID === id && event.eventType === eventType) {
>-          Logger.log(`Correct event DOMNode id: ${id}`);
>+        // If event's accessible does not match expected event type and DOMNode
>+        // id or accessible, skip thie event.

The comment isn't about what is just below it.
>+        if (event.eventType !== eventType) {
>+          return;
>+        }
>+
>+        let acc = event.accessible;
>+        let id = getAccessibleDOMNodeID(acc);

if you call observers before shutting down the proxies in hide events that call is not ok because it triggers sync IPC to the child which could change the tree.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1272491 patch v2

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8754522 [details] [diff] [review]
1272491 patch v2

>+  if (nsCoreUtils::AccEventObserversExist()) {
>+    uint32_t type = nsIAccessibleEvent::EVENT_HIDE;
>+    xpcAccessibleGeneric* xpcAcc = GetXPCAccessible(root);
>+    xpcAccessibleGeneric* xpcParent = GetXPCAccessible(parent);
>+    xpcAccessibleGeneric* xpcNext = root->NextSibling() ?
>+      GetXPCAccessible(root->NextSibling()) : nullptr;
>+    xpcAccessibleGeneric* xpcPrev = root->PrevSibling() ?
>+      GetXPCAccessible(root->PrevSibling()) : nullptr;

Net/PrevSibling aren't the cheapest some local vars might be good, but its only for xpcom stuff so whatever I guess.

>-    <div id="div"></div>
>+    <div id="to-hide"></div>
>     <div id="next"></div>
>-  </div>`, function*(browser) {
>-  let onHide = waitForEvent(EVENT_HIDE, 'div');
>-  yield invokeSetStyle(browser, 'div', 'visibility', 'hidden');
>-  let event = yield onHide;
>-  let hideEvent = event.QueryInterface(Ci.nsIAccessibleHideEvent);
>+  </div>`,
>+  function*(browser, accDoc) {

{ on same line as declaration is annoying

also it'd be a lot easier to read with the renaming split into another patch.

>-  let onReorder = waitForEvent(EVENT_REORDER, id);
>+  let onReorderEvent = waitForEvent(EVENT_REORDER, id);

I'm not sure that's a better name, its not an event its a promise that may become an event or something right? but whatever I guess.

> /**
>- * A helper function that waits for an accessible event of certain type that
>- * belongs to a certain DOMNode (defined by its id).
>- * @param  {String}  id         expected content element id for the event
>- * @param  {Number}  eventType  expected accessible event type
>- * @return {Promise}            promise that resolves to an event
>+ * A helper function that waits for an accessible event of the given type with
>+ * the given target (defined by its id or accessible).
that's not true right? it returns a promise that will wait, but it does not itself wait.

>+        // If event's accessible does not match expected DOMNode id or
>+        // accessible, skip thie event.

typo

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddaa43e26575
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb567ace229f

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddaa43e26575
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb567ace229f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ddaa43e26575
https://hg.mozilla.org/mozilla-central/rev/eb567ace229f