Closed Bug 1390409 Opened 2 years ago Closed 2 years ago

stylo: need to notify accessibility service when visibility property flips during restyle

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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
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
Priority: -- → P2
Duplicate of this bug: 1384009
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).
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.
(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?
(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.
Summary: stylo: need to notify accessibility service when visible property flips during restyle → stylo: need to notify accessibility service when visibility property flips during restyle
Blocks: 1391141
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.
(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.
I'll look into this.
Assignee: nobody → xidorn+moz
Comment on attachment 8901776 [details]
Bug 1390409 part 1 - Remove unused ServoTraversalFlags param from ServoRestyleManager::ProcessPostTraversal.

https://reviewboard.mozilla.org/r/173204/#review178536
Attachment #8901776 - Flags: review?(emilio) → 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.
Attachment #8901777 - Flags: review?(emilio) → review+
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.
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.
Attachment #8901778 - Flags: review?(emilio) → 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.
Attachment #8901779 - Flags: review?(emilio) → review+
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.
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 :)
(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/
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.
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
You need to log in before you can comment on or make changes to this bug.