Bug 57685 – AX: VoiceOver cannot use AXTextMarker calls when user-select:none

• Status: NEW
• Component: Accessibility

[chris fleizach]

If an element has user-select:none, VO cannot use any of the AXTextMarker based calls it uses. This is because the those methods rely on Position calls which will return results that stop Positions from being found ahead or behind from the current Position

This affects two scenarions now
1) If a mail message contains user-select:none, a VO user cannot navigate it at all
2) If a webpage has this, a VO user cannot navigate by line option

[chris fleizach]

Created attachment 87948 [details]
Patch

[Darin Adler]

Comment on attachment 87948 [details]
Patch

We should look for a way to do this without this global variable hack. I see that the problem is serious.

[chris fleizach]

(In reply to comment #2)
> (From update of attachment 87948 [details])
> We should look for a way to do this without this global variable hack. I see that the problem is serious.

Any ideas?

(In reply to comment #2)
> (From update of attachment 87948 [details])
> We should look for a way to do this without this global variable hack. I see that the problem is serious.

It would be great if the fix for this bug also helped the text search APIs to work with non-selectable text.

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 87948 [details] [details])
> > We should look for a way to do this without this global variable hack. I see that the problem is serious.
> 
> It would be great if the fix for this bug also helped the text search APIs to work with non-selectable text.

The global hack would allow you to do that (just turn it off before searching)

Another solution I was toying with, but because ugly, is trying to set IgnoreUserSelectionOptions on a Position, so that when it went to check that ability, it would see the position ignored that option. Unfortunately, that because pretty ugly, because that bit needs to be copied to the new Position when you do pos.advance() or pos.next() or other such operations.

If anyone else has other ideas, feel free to chime in

[Ryosuke Niwa]

It seems odd that Position that lives in WebCore/dom is aware of user-select: none. That seems like editing-level concept and shouldn't be affecting DOM concept.  In the long term, we should probably move that concept or the concept of canonicalization from Position altogether.

I feel like AX code should always be using RenderedPosition as proposed in https://lists.webkit.org/pipermail/webkit-dev/2011-February/015842.html

[Enrica Casucci]

I agree with Darin that using a global hack is not the right solution. It is particularly bad because the global is set for the Position class (that is the one implementing isCandidate that internally uses nodeIsUserSelectNone, the method affected by the global) whereas the rest of the Accessibility related code only deals with VisiblePosition.
It makes the code even more obscure.
Even though I agree with Ryosuke that we currently don't have a position related class that suites our needs in this case, I also understand that this is an important fix and cannot wait for a major refactoring.

My recommendation would be to extend the VisiblePosition class providing a new constructor that is able to produce visible positions that will consider as candidate also nodes with user-select-none.
It is more work to implement it this way but it is, in my opinion, a lot cleaner. As a consequence, you would need to extend the Position class as well to make it aware of the new behavior of the isCandidate() method you want.

[chris fleizach]

(In reply to comment #7)
> I agree with Darin that using a global hack is not the right solution. It is particularly bad because the global is set for the Position class (that is the one implementing isCandidate that internally uses nodeIsUserSelectNone, the method affected by the global) whereas the rest of the Accessibility related code only deals with VisiblePosition.
> It makes the code even more obscure.
> Even though I agree with Ryosuke that we currently don't have a position related class that suites our needs in this case, I also understand that this is an important fix and cannot wait for a major refactoring.
> 
> My recommendation would be to extend the VisiblePosition class providing a new constructor that is able to produce visible positions that will consider as candidate also nodes with user-select-none.
> It is more work to implement it this way but it is, in my opinion, a lot cleaner. As a consequence, you would need to extend the Position class as well to make it aware of the new behavior of the isCandidate() method you want.

I was going down this path as well before, but then when I saw that by calling things like p.next(), you'd have to propagate that information to Positions that are also created, I went for the global method... I will try to refactor using your suggestion and see if i can make it work

[Eric Seidel (no email)]

Comment on attachment 87948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87948&action=review

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2544
> +    Position::setUserSelectNoneIgnored(true);

This should be a RIIA instead, and then this is a 3 line patch instead of 300. :)

[Eric Seidel (no email)]

http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization RAII rather.

[chris fleizach]

Created attachment 209141 [details]
patch

[chris fleizach]

<rdar://problem/9223313>

[WebKit Commit Bot]

Attachment 209141 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/text-marker-with-user-select-none-expected.txt', u'LayoutTests/platform/mac/accessibility/text-marker-with-user-select-none.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/accessibility/AXObjectCache.cpp', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp', u'Source/WebCore/dom/Position.cpp', u'Source/WebCore/dom/Position.h', u'Source/WebCore/dom/PositionIterator.cpp', u'Source/WebCore/dom/PositionIterator.h', u'Source/WebCore/editing/VisiblePosition.cpp', u'Source/WebCore/editing/VisibleUnits.cpp', u'Source/WebCore/editing/htmlediting.cpp', u'Source/WebCore/editing/htmlediting.h', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/rendering/RenderBR.cpp', u'Source/WebCore/rendering/RenderBR.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderBox.h', u'Source/WebCore/rendering/RenderFileUploadControl.cpp', u'Source/WebCore/rendering/RenderFileUploadControl.h', u'Source/WebCore/rendering/RenderInline.cpp', u'Source/WebCore/rendering/RenderInline.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderReplaced.cpp', u'Source/WebCore/rendering/RenderReplaced.h', u'Source/WebCore/rendering/RenderText.cpp', u'Source/WebCore/rendering/RenderText.h', u'Source/WebCore/rendering/svg/RenderSVGInlineText.cpp', u'Source/WebCore/rendering/svg/RenderSVGInlineText.h', u'Source/WebCore/rendering/svg/RenderSVGText.cpp', u'Source/WebCore/rendering/svg/RenderSVGText.h']" exit_code: 1
Source/WebCore/rendering/RenderInline.h:150:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/svg/RenderSVGText.h:67:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderObject.h:724:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderObject.h:725:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBox.h:501:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBR.h:54:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Position.h:84:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Position.h:88:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Position.h:91:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Position.h:92:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Position.h:96:  The parameter name "userSelectNone" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderFileUploadControl.h:57:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:233:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderReplaced.h:78:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderText.h:67:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/svg/RenderSVGInlineText.h:60:  The parameter name "userSelectNoneType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 16 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 209142 [details]
patch

[Doug Russell]

Created attachment 252401 [details]
patch

Implement new methods in Position and PositionIterator to consult instead of Position::nodeIsUserSelectNone() when considering if a position is considered a candidate:
bool Position::positionIsUserSelectNone(Node* node) const
bool PositionIterator::positionIteratorNodeIsUserSelectNone(Node* node) const

These methods consult AXObjectCache::accessibilityEnhancedUserInterfaceEnabled() and return false when enhanced to allow positions to move into user-select:none regions.

FrameSelection::setSelection() now validates selections to only allow selections with end points that reside in user-select:none regions if they are not range selections.

DOMSelection::visibleSelection() now validates for caret selections that are in user-select:none regions and returns a null selection.

[Doug Russell]

Created attachment 252402 [details]
patch

Fix a style error

[Build Bot]

Comment on attachment 252402 [details]
patch

Attachment 252402 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6381551117926400

New failing tests:
fast/forms/input-select-webkit-user-select-none.html

[Build Bot]

Created attachment 252407 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 252402 [details]
patch

Attachment 252402 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5314513737875456

New failing tests:
fast/forms/input-select-webkit-user-select-none.html

[Build Bot]

Created attachment 252408 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[chris fleizach]

Comment on attachment 252402 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252402&action=review

> Source/WebCore/dom/PositionIterator.cpp:147
> +bool PositionIterator::positionIteratorNodeIsUserSelectNone(Node* node) const

doesn't look like this method is needed and that we can use the Position version exclusively

> Source/WebCore/editing/FrameSelection.cpp:342
> +    if (selection.isRange()) {

does this allow for a selection that isCaret only to proceed?

> Source/WebCore/page/DOMSelection.cpp:80
> +    if (!Position::nodeIsUserSelectNone(node))

i think this if could be combined with the 1 above

[Doug Russell]

(In reply to comment #21)
> Comment on attachment 252402 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252402&action=review
> 
> > Source/WebCore/dom/PositionIterator.cpp:147
> > +bool PositionIterator::positionIteratorNodeIsUserSelectNone(Node* node) const
> 
> doesn't look like this method is needed and that we can use the Position
> version exclusively

Makes sense

> 
> > Source/WebCore/editing/FrameSelection.cpp:342
> > +    if (selection.isRange()) {
> 
> does this allow for a selection that isCaret only to proceed?

Yes

> 
> > Source/WebCore/page/DOMSelection.cpp:80
> > +    if (!Position::nodeIsUserSelectNone(node))
> 
> i think this if could be combined with the 1 above

Yup

[Ryosuke Niwa]

Comment on attachment 252402 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252402&action=review

> Source/WebCore/dom/Position.cpp:977
> +bool Position::positionIsUserSelectNone(Node* node) const
> +{
> +    if (AXObjectCache::accessibilityEnhancedUserInterfaceEnabled())
> +        return false;
> +    return nodeIsUserSelectNone(node);
> +}

I don't think we should be changing the behavior of Position globally like this.
We should have a flag passed in from AX code to Position and other objects AX code uses instead.
We already have code in htmlediting.cpp to deal with aria role. See isEditableToAccessibility.

[Doug Russell]

(In reply to comment #23)
> Comment on attachment 252402 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252402&action=review
> 
> > Source/WebCore/dom/Position.cpp:977
> > +bool Position::positionIsUserSelectNone(Node* node) const
> > +{
> > +    if (AXObjectCache::accessibilityEnhancedUserInterfaceEnabled())
> > +        return false;
> > +    return nodeIsUserSelectNone(node);
> > +}
> 
> I don't think we should be changing the behavior of Position globally like
> this.
> We should have a flag passed in from AX code to Position and other objects
> AX code uses instead.
> We already have code in htmlediting.cpp to deal with aria role. See
> isEditableToAccessibility.

My initial pass on this was to update Chris's patch which did just that, but I had two concerns:

1. It ends up touching a lot of code. Which is tractable, but unfortunate.
2. It's important this behavior ends when the assistive technology disconnects and making sure there aren't Position values that originated from accessibility logic, but persisted past the assistive app disconnecting seems tricky.

[chris fleizach]

Comment on attachment 209142 [details]
patch

fixed with other bugs

[Siri Gubba]

Able to reproduce this bug for Safari 13 and MACOS 10.14