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
Created attachment 87948 [details] Patch
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.
(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.
(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
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
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.
(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
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. :)
http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization RAII rather.
Created attachment 209141 [details] patch
<rdar://problem/9223313>
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.
Created attachment 209142 [details] patch
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.
Created attachment 252402 [details] patch Fix a style error
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
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
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
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
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
(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
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.
(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.
Comment on attachment 209142 [details] patch fixed with other bugs
Able to reproduce this bug for Safari 13 and MACOS 10.14