Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 7 years ago

At least accessible/tests/mochitest/events/test_aria_menu.html checks that an accessibility event should be invoked when value of visibility property changes between visible and hidden.

In Gecko's style system, this is handled in ElementRestyler::InitializeAccessibilityNotifications [1] and ElementRestyler::SendAccessibilityNotifications [2].


[1] https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/base/GeckoRestyleManager.cpp#3391-3430
[2] https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/base/GeckoRestyleManager.cpp#3505-3539

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 1 • 7 years ago

It seems all failures in a11y task are actually just this bug. That includes
* accessible/tests/mochitest/events/test_aria_menu.html
* accessible/tests/mochitest/events/test_docload.html
* accessible/tests/mochitest/events/test_mutation.html
* accessible/tests/mochitest/treeupdate/test_visibility.html

	Emilio Cobos Álvarez (:emilio)
	Comment 3 • 7 years ago

I wonder if it would make sense to move this code to nsIFrame::DidSetStyleContext, seems that it'd be nicer (less stuff to deal with).

	Bobby Holley (:bholley)
	Comment 4 • 7 years ago

In general I'd rather move things into change hint handling than DidSetStyleContext, since the eventual hope is to move all the DidSetStyleContext stuff to change hint handling (to get rid of the post-traversal). So let's do that if we can to avoid creating more work down the line, but DidSetStyleContext is a fine fallback if that's hard for some reason.

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 7 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> In general I'd rather move things into change hint handling than
> DidSetStyleContext, since the eventual hope is to move all the
> DidSetStyleContext stuff to change hint handling (to get rid of the
> post-traversal). So let's do that if we can to avoid creating more work down
> the line, but DidSetStyleContext is a fine fallback if that's hard for some
> reason.

Yeah, the problem is that that code needs to run for every restyled frame IIUC... Maybe we can filter out on the ones that caused a given set of change hints, but I don't know how that'd play with the handled for descendants thing, presumably not super-well?

	Bobby Holley (:bholley)
	Comment 6 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> > In general I'd rather move things into change hint handling than
> > DidSetStyleContext, since the eventual hope is to move all the
> > DidSetStyleContext stuff to change hint handling (to get rid of the
> > post-traversal). So let's do that if we can to avoid creating more work down
> > the line, but DidSetStyleContext is a fine fallback if that's hard for some
> > reason.
> 
> Yeah, the problem is that that code needs to run for every restyled frame
> IIUC... Maybe we can filter out on the ones that caused a given set of
> change hints, but I don't know how that'd play with the handled for
> descendants thing, presumably not super-well?

Hm. I guess we can just put it in DidSetStyleContext for now and then deal with it later.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 7 • 7 years ago

How can moving this into nsIFrame::DidSetStyleContext work actually? It doesn't seem to handle descendant thing either.

What we need here is that, if an element has its visibility changed from visible to hidden, all its children which are still visible need some extra handling.

If we do it in nsIFrame::DidSetStyleContext of the parent, we don't know what children need this handling because they haven't got their new style context. If we do it in that of children, if the style context isn't changed for the children, we may not get to DidSetStyleContext at all.

I'm also concerned about that DidSetStyleContext can happen multiple times for a given Element, so we would need to check whether we are dealing with the primary frame of a given element in that case.

It seems to me that we probably need to do this in ServoRestyleManager::ProcessPostTraversal.

	Bobby Holley (:bholley)
	Comment 8 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> It seems to me that we probably need to do this in
> ServoRestyleManager::ProcessPostTraversal.

Ok - let's just make sure we don't do any extra work in the cases where accessibility is inactive.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 9 • 7 years ago

I'll look into this.

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8901776 [details]
Bug 1390409 part 1 - Remove unused ServoTraversalFlags param from ServoRestyleManager::ProcessPostTraversal.

https://reviewboard.mozilla.org/r/173204/#review178536

	Emilio Cobos Álvarez (:emilio)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8901777 [details]
Bug 1390409 part 2 - Replace aParentWasRestyled with a bitflag.

https://reviewboard.mozilla.org/r/173206/#review178540

::: layout/base/ServoRestyleManager.cpp:832
(Diff revision 1)
>    const bool traverseTextChildren =
>      wasRestyled || aElement->HasFlag(NODE_DESCENDANTS_NEED_FRAMES);
>    bool recreatedAnyContext = wasRestyled;
> +  ServoPostTraversalFlags childrenFlags =
> +    wasRestyled ? ServoPostTraversalFlags::ParentWasRestyled
> +                : ServoPostTraversalFlags::Empty;

nit: This could move to the inner block.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 16 • 7 years ago
mozreview-review-reply

Comment on attachment 8901777 [details]
Bug 1390409 part 2 - Replace aParentWasRestyled with a bitflag.

https://reviewboard.mozilla.org/r/173206/#review178540

> nit: This could move to the inner block.

It will be moved in part 3, so doesn't really matter here.

	Emilio Cobos Álvarez (:emilio)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8901778 [details]
Bug 1390409 part 3 - Send a11y notification for visibility changes.

https://reviewboard.mozilla.org/r/173208/#review178544

Looks great, Xidorn, thanks for fixing this! :)

::: layout/base/ServoRestyleManager.cpp:671
(Diff revision 1)
> +                      Element* aElement,
> +                      nsStyleContext* aOldStyleContext,
> +                      nsStyleContext* aNewStyleContext,
> +                      ServoPostTraversalFlags aFlags)
> +{
> +  using Flags = ServoPostTraversalFlags;

Let's assert here that the two accessibility bits are mutually exclussive?

::: layout/base/ServoRestyleManager.cpp:673
(Diff revision 1)
> +                      nsStyleContext* aNewStyleContext,
> +                      ServoPostTraversalFlags aFlags)
> +{
> +  using Flags = ServoPostTraversalFlags;
> +
> +#ifdef ACCESSIBILITY

Do we support non-accessibility builds? It'd be nice to remove the ifdef if not...

::: layout/base/ServoRestyleManager.cpp:713
(Diff revision 1)
> +      // We are adding the subtree. Accessibility service would handle
> +      // descendants, so we should just skip them from notifying.
> +      return Flags::SkipA11yNotifications;
> +    }
> +    // Remove the subtree from this invisible element, and ask any
> +    // shown descendant to add them back themselves.

nit: I'd use `to add themselves back`.

::: layout/base/ServoRestyleManager.cpp:835
(Diff revision 1)
>    RefPtr<ServoStyleContext> upToDateContext =
>      wasRestyled
>        ? aRestyleState.StyleSet().ResolveServoStyle(aElement)
>        : oldStyleContext;
>  
> +  ServoPostTraversalFlags childrenFlags =

Disregard the "move to the inner block" bit of the first commit.

::: layout/base/ServoRestyleManager.cpp:895
(Diff revision 1)
>      AddLayerChangesForAnimation(
>        styleFrame, aElement, aRestyleState.ChangeList());
> +
> +    childrenFlags |= SendA11yNotifications(mPresContext, aElement,
> +                                           oldStyleContext,
> +                                           upToDateContext, aFlags);

Maybe store the `SendA11yNotifications` result in a temporary, and assert it only contains accessibility bits? Maybe that's really over-paranoid.

	Emilio Cobos Álvarez (:emilio)
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8901779 [details]
Bug 1390409 part 4 - Enable tests which were skipped because of this bug.

https://reviewboard.mozilla.org/r/173210/#review178548

Nice! I didn't know this affected so many tests.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 19 • 7 years ago
mozreview-review-reply

Comment on attachment 8901778 [details]
Bug 1390409 part 3 - Send a11y notification for visibility changes.

https://reviewboard.mozilla.org/r/173208/#review178544

> Do we support non-accessibility builds? It'd be nice to remove the ifdef if not...

I think we do. We have `#ifdef ACCESSIBILITY` around all the related places. Actually, IIRC the default local build is done without accessbility included. You need to explicitly add `ac_add_options --enable-accessibility` to enable that.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 20 • 7 years ago
mozreview-review-reply

Comment on attachment 8901779 [details]
Bug 1390409 part 4 - Enable tests which were skipped because of this bug.

https://reviewboard.mozilla.org/r/173210/#review178548

And as far as I know, this is also the only bug stylo has for accessibility. So with this fixed, we are really accessible now :)

	Emilio Cobos Álvarez (:emilio)
	Comment 21 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #20)
> Comment on attachment 8901779 [details]
> Bug 1390409 part 4 - Enable tests which were skipped because of this bug.
> 
> https://reviewboard.mozilla.org/r/173210/#review178548
> 
> And as far as I know, this is also the only bug stylo has for accessibility.
> So with this fixed, we are really accessible now :)

\o/

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 22 • 7 years ago
mozreview-review-reply

Comment on attachment 8901778 [details]
Bug 1390409 part 3 - Send a11y notification for visibility changes.

https://reviewboard.mozilla.org/r/173208/#review178544

> Maybe store the `SendA11yNotifications` result in a temporary, and assert it only contains accessibility bits? Maybe that's really over-paranoid.

It doesn't seem to me doing that has any benefit... We would be introducing an additional variable, which doesn't really make much sense in my opinion... I'd prefer to just keep this code as-is.

	Pulsebot
	Comment 25 • 7 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9900e7757a4f
part 1 - Remove unused ServoTraversalFlags param from ServoRestyleManager::ProcessPostTraversal. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2a54187a362e
part 2 - Replace aParentWasRestyled with a bitflag. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a5c00f0df5be
part 3 - Send a11y notification for visibility changes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5acb54a4d447
part 4 - Enable tests which were skipped because of this bug. r=emilio

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 26 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9900e7757a4f
https://hg.mozilla.org/mozilla-central/rev/2a54187a362e
https://hg.mozilla.org/mozilla-central/rev/a5c00f0df5be
https://hg.mozilla.org/mozilla-central/rev/5acb54a4d447