Bug 71263 – Report correct line number for non-native editable text elements.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Alice Boxhall]

Currently an incorrect line number is reported as the code in question looks for a native root editable element.

[Alice Boxhall]

Created attachment 113126 [details]
Patch

[Alice Boxhall]

Ok, here's my first pass; I expect it'll need a great deal of work.

[Alice Boxhall]

Chris, I modified textarea-insertion-point-line-number.html because I didn't think it was actually testing what it was supposed to test. Could you take a look and tell me whether it's now right?

[Early Warning System Bot]

Comment on attachment 113126 [details]
Patch

Attachment 113126 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10240947

[WebKit Review Bot]

Comment on attachment 113126 [details]
Patch

Attachment 113126 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10245528

[Collabora GTK+ EWS bot]

Comment on attachment 113126 [details]
Patch

Attachment 113126 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10240949

[chris fleizach]

Can you explain more what this patch does and what the problem is

Are you saying the insertionPointLineNumber method is not working? Or just that the layout test isn't working?

[Alice Boxhall]

Created attachment 113268 [details]
Patch

[Alice Boxhall]

Hopefully fixed the issue that was causing the EWS breakages.

[Alice Boxhall]

(In reply to comment #7)
> Can you explain more what this patch does and what the problem is
> 
> Are you saying the insertionPointLineNumber method is not working? Or just that the layout test isn't working?

Hm -- it does somewhat conflate two things:
- insertionPointLineNumber is reported incorrectly for _non-native_ text fields (i.e. ARIA role=textbox) because the code which calculates it only works for native text fields.
- while I was fixing the above, I broke the accessibility/textarea-insertion-point-line-number.html in a way that I couldn't understand because I found AccessibilityObject::lineForPosition hard to follow, so I tried to modify it to make the logic clearer. Additionally, I don't think the test was correctly testing the behaviour for having focus outside the textarea which is reporting the line number, so I modified the test to reflect what I thought it should be doing -- please let me know if I've misunderstood, but the previous behaviour seemed to be testing something other than what the comment said, so I made it reflect the comment.

I am happy to pull out the second part of the above into a separate patch if you like.

[WebKit Review Bot]

Comment on attachment 113268 [details]
Patch

Attachment 113268 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10257132

New failing tests:
editing/pasteboard/paste-delete-insertion-position-skip-paragraph.html
editing/selection/4960116.html

[chris fleizach]

Comment on attachment 113268 [details]
Patch

My overall sense is that this is a risky change in order to make a non-editable div appear as a text box role so that it can report 0 as its insertionPointLineNumber

It seems like you could return 0 for insertionPointLineNumber if aria-multiline = false and that would achieve the same result, since the insertionPointLineNumber should always be 0 for aria-multiline=false.

Does this patch attempt to solve other problems?

[Alice Boxhall]

(In reply to comment #12)
> (From update of attachment 113268 [details])
> My overall sense is that this is a risky change in order to make a non-editable div appear as a text box role so that it can report 0 as its insertionPointLineNumber
> 
> It seems like you could return 0 for insertionPointLineNumber if aria-multiline = false and that would achieve the same result, since the insertionPointLineNumber should always be 0 for aria-multiline=false.
> 
> Does this patch attempt to solve other problems?

It seems like that is a hack that I'll eventually have to fix when I implement multiline=true; why not just do it right the first time?

[chris fleizach]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 113268 [details] [details])
> > My overall sense is that this is a risky change in order to make a non-editable div appear as a text box role so that it can report 0 as its insertionPointLineNumber
> > 
> > It seems like you could return 0 for insertionPointLineNumber if aria-multiline = false and that would achieve the same result, since the insertionPointLineNumber should always be 0 for aria-multiline=false.
> > 
> > Does this patch attempt to solve other problems?
> 
> It seems like that is a hack that I'll eventually have to fix when I implement multiline=true; why not just do it right the first time?

Your layout test only tests that aspect of it, so it's hard to know what else this patch might address.

I'm also unclear what is motivating the need to return insertionPointLineNumbers for non-editable divs. Are people making plain old <divs> into text boxes and expecting different behavior from VoiceOver?

[Dominic Mazzoni]

> Are people making plain old <divs> into text boxes?

Definitely yes! Google Docs is a big motivating example for us, but other examples include:
* http://ace.ajax.org/build/kitchen-sink.html
* http://www.ymacs.org/demo/

Also do a web search for "canvas text editor" and you'll find there's lots of interest there too - and it'd be nice to provide a way to make those accessible, too.

> and expecting different behavior from VoiceOver?

Not yet, but fundamentally these examples aren't that different than contentEditable - the only difference is that the programmer is taking over all of the text, cursor and selection manipulation rather than letting the browser handle it.

As for this patch, I wonder if there's a way to make it a little cleaner.

One idea: rather than modifying Node, htmlediting, and VisiblePosition to understand the concept of an AX editable root, maybe some of those methods could just be refactored to take an editing root as an argument? That way you could query them for information about the current line, etc. and pass the root you want it computed relative to.

[Alice Boxhall]

I created bug 71348 to pull out just the code cleanup changes.

[Alice Boxhall]

+darin, enrica, justing

I'm honestly not sure what the best approach is here; looking for advice from editing folks. I discussed the vague idea with Ryosuke initially, but we may have not been on the same page.

[Ryosuke Niwa]

Comment on attachment 113268 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +

You definitely need to explain what changes you're making here. r- due to lack of explanation.

> Source/WebCore/accessibility/AXObjectCache.cpp:654
> +    if (rootAXEditableElementForNode(node))
> +        return true;
> +
> +    return false;

This function should be one liner:
return rootAXEditableElementForNode(node);

Also, please make this function inline (put definition in AXObjectCache.h) to avoid an extra function call.

> Source/WebCore/accessibility/AXObjectCache.cpp:662
> +    const Element* e = node->isElementNode() ? toElement(node) : node->parentElement();

Please don't use one-letter variable.

> Source/WebCore/accessibility/AXObjectCache.cpp:663
> +    for ( ; e && e != rootEditableElement; e = e->parentElement()) {

I don't think we should stop at root editable element. e.g. consider <div role="textbox"><span contenteditable>hello</span></div> where node is of "hello".

> Source/WebCore/accessibility/AXObjectCache.cpp:665
> +            result = e;

Please do an early return here instead.

> Source/WebCore/accessibility/AXObjectCache.cpp:667
> +        if (e->hasTagName(bodyTag))
> +            break;

It seems like this condition should be in the for.

> Source/WebCore/accessibility/AXObjectCache.cpp:681
> +    const AccessibilityObject* axObjectForNode = getOrCreate(node->renderer());

It seems odd that asking whether a node is a text control ends up creating an accessibility object for it.

> Source/WebCore/accessibility/AXObjectCache.h:108
> +    // Elements which act as editable text need to be treated specially by some non-AX code.

I don't think we need this comment.

> Source/WebCore/accessibility/AXObjectCache.h:112
> +    bool hasAXEditingElements() const { return m_hasAXEditingElements; }
> +    const Element* rootAXEditableElementForNode(const Node*);
> +    bool nodeIsAXEditable(const Node*);
> +    bool nodeIsTextControl(const Node*);

Can we give these functions more descriptive names such as nodeActAsTextControl, and rootAriaTextControlElement?

> Source/WebCore/accessibility/AccessibilityObject.cpp:945
> +    // if the position is not in the same editable region as this AX object, return -1

Instead of adding this comment, can you add const int to describe this? e.g. NotFound.

> Source/WebCore/accessibility/AccessibilityObject.cpp:951
> +    VisiblePosition currentVisiblePos = visiblePos, savedVisiblePos;

Please don't declare two variables in one line.

> Source/WebCore/dom/Node.cpp:1654
> +    if (rule == CannotCrossAXEditingBoundary && document()->axObjectCache()->hasAXEditingElements())
> +        return rootAXEditableElement();
> +
> +    return rootEditableElement();

Can this be an inline function?

> Source/WebCore/dom/Node.cpp:1658
> +Element* Node::rootAXEditableElement() const
> +{

I don't really like this function name. It only tells me that it's related to accessibility code. How about rootAriaTextControlElement?

> Source/WebCore/dom/Node.cpp:1659
> +    return const_cast<Element*>(document()->axObjectCache()->rootAXEditableElementForNode(this));

Can this be defined in Node.h or will that introduce new header dependency?

> Source/WebCore/dom/Node.h:350
> +    bool rendererIsAXEditable() const { return rendererIsEditable(AXEditable); }

Again, I'm not digging the function name. How about rendererIsInAriaTextControl?

> Source/WebCore/dom/Node.h:702
> +    enum EditableLevel { Editable, RichlyEditable, AXEditable };

How about InAriaTextControl or AriaTextControlEditable?

> Source/WebCore/editing/EditingBoundary.h:34
> +    CannotCrossAXEditingBoundary,

Ditto about AX.

> Source/WebCore/editing/FrameSelection.cpp:1023
> -        next = (direction == DirectionUp ? previousLinePosition : nextLinePosition)(p, xPos);
> +        if (direction == DirectionUp)
> +            next = previousLinePosition(p, xPos);
> +        else
> +            next = nextLinePosition(p, xPos);
> +

Could you put this change into a separate patch?

> Source/WebCore/editing/VisiblePosition.cpp:445
> +
> +    Node* highestRoot = highestEditableRootRespectingAXEditing(deepEquivalent());

I don't think we want these functions to magically change behaviors. Callers of these functions should indicate which editing boundary this function should respect.

> Source/WebCore/editing/VisiblePosition.cpp:454
> -    if (highestEditableRoot(pos.deepEquivalent()) == highestRoot)
> +    if (highestEditableRootRespectingAXEditing(pos.deepEquivalent()) == highestRoot)

Ditto.

> Source/WebCore/editing/VisiblePosition.cpp:471
> -    Node* highestRoot = highestEditableRoot(deepEquivalent());
> +    Node* highestRoot = highestEditableRootRespectingAXEditing(deepEquivalent());

Ditto.

> Source/WebCore/editing/VisiblePosition.cpp:480
> -    if (highestEditableRoot(pos.deepEquivalent()) == highestRoot)
> +    if (highestEditableRootRespectingAXEditing(pos.deepEquivalent()) == highestRoot)

Ditto.

> Source/WebCore/editing/VisiblePosition.h:84
>      Element* rootEditableElement() const { return m_deepPosition.isNotNull() ? m_deepPosition.deprecatedNode()->rootEditableElement() : 0; }
> +    Element* rootEditableElement(EditingBoundaryCrossingRule rule) const { return m_deepPosition.isNotNull() ? m_deepPosition.deprecatedNode()->rootEditableElement(rule) : 0; };

We should be calling containerNode() instead of deprecatedNode()

> Source/WebCore/editing/htmlediting.cpp:129
> -        if (node->rendererIsEditable())
> +        if (nodeIsEditableForRule(node, rule))

We should probably convert EditingBoundaryCrossingRule to EditableLevel here instead of calling different functions.

[Alice Boxhall]

Created attachment 113819 [details]
Patch

[Alice Boxhall]

Comment on attachment 113268 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +
> 
> You definitely need to explain what changes you're making here. r- due to lack of explanation.

Will fix once changes are more bedded down.

>> Source/WebCore/accessibility/AXObjectCache.cpp:654
>> +    return false;
> 
> This function should be one liner:
> return rootAXEditableElementForNode(node);
> 
> Also, please make this function inline (put definition in AXObjectCache.h) to avoid an extra function call.

Done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:662
>> +    const Element* e = node->isElementNode() ? toElement(node) : node->parentElement();
> 
> Please don't use one-letter variable.

It's a loop variable; if I make the name longer, the for loop below becomes hard to read.

>> Source/WebCore/accessibility/AXObjectCache.cpp:663
>> +    for ( ; e && e != rootEditableElement; e = e->parentElement()) {
> 
> I don't think we should stop at root editable element. e.g. consider <div role="textbox"><span contenteditable>hello</span></div> where node is of "hello".

Done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:665
>> +            result = e;
> 
> Please do an early return here instead.

This is not the end condition; I want to find the _root_ element, which may be higher up (as in the example you gave).

>> Source/WebCore/accessibility/AXObjectCache.cpp:667
>> +            break;
> 
> It seems like this condition should be in the for.

Done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:681
>> +    const AccessibilityObject* axObjectForNode = getOrCreate(node->renderer());
> 
> It seems odd that asking whether a node is a text control ends up creating an accessibility object for it.

Yeah, I agree.

I don't think I can assume that an AccessibilityObject will exist for any node which is a text control, and I want to use AccessibilityObject::isTextControl, so that's why it ended up being this way. Perhaps there are some more early outs I can use so that I avoid creating AccessibilityObjects for nodes which are definitely not going to end up being text controls?

>> Source/WebCore/accessibility/AXObjectCache.h:108
>> +    // Elements which act as editable text need to be treated specially by some non-AX code.
> 
> I don't think we need this comment.

Done.

>> Source/WebCore/accessibility/AXObjectCache.h:112
>> +    bool nodeIsTextControl(const Node*);
> 
> Can we give these functions more descriptive names such as nodeActAsTextControl, and rootAriaTextControlElement?

Hm... those aren't strictly accurate, as these tests encompass both editable-to-screenreader-only and native editable. Will have a think about a better name.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:945
>> +    // if the position is not in the same editable region as this AX object, return -1
> 
> Instead of adding this comment, can you add const int to describe this? e.g. NotFound.

I'm waiting to hear back on bug 71348 whether this is actually the correct behaviour; it was vaguely implied by the old code, but it didn't work quite the same as this (but I think it was incorrect). I will make this change there if it is the correct behaviour.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:951
>> +    VisiblePosition currentVisiblePos = visiblePos, savedVisiblePos;
> 
> Please don't declare two variables in one line.

Done.

>> Source/WebCore/dom/Node.cpp:1654
>> +    return rootEditableElement();
> 
> Can this be an inline function?

I don't know how to judge that, sorry. Do you think it can, as is?

>> Source/WebCore/dom/Node.cpp:1658
>> +{
> 
> I don't really like this function name. It only tells me that it's related to accessibility code. How about rootAriaTextControlElement?

As above, that is not strictly accurate, as it is actually any element which is treated as editable by a11y code, whether native editable or not. Still thinking about a better way to express that.

>> Source/WebCore/dom/Node.cpp:1659
>> +    return const_cast<Element*>(document()->axObjectCache()->rootAXEditableElementForNode(this));
> 
> Can this be defined in Node.h or will that introduce new header dependency?

AXObjectCache.h is not included in Node.h.

>> Source/WebCore/dom/Node.h:350
>> +    bool rendererIsAXEditable() const { return rendererIsEditable(AXEditable); }
> 
> Again, I'm not digging the function name. How about rendererIsInAriaTextControl?

As above.

>> Source/WebCore/dom/Node.h:702
>> +    enum EditableLevel { Editable, RichlyEditable, AXEditable };
> 
> How about InAriaTextControl or AriaTextControlEditable?

As above.

>> Source/WebCore/editing/EditingBoundary.h:34
>> +    CannotCrossAXEditingBoundary,
> 
> Ditto about AX.

As above.

>> Source/WebCore/editing/FrameSelection.cpp:1023
>> +
> 
> Could you put this change into a separate patch?

Ok. Bug 71646 created.

>> Source/WebCore/editing/VisiblePosition.cpp:445
>> +    Node* highestRoot = highestEditableRootRespectingAXEditing(deepEquivalent());
> 
> I don't think we want these functions to magically change behaviors. Callers of these functions should indicate which editing boundary this function should respect.

Respecting AX editing boundary only makes sense if a11y is turned on and ARIA editable elements are present, which this method checks. Should I move that check into highestEditableRoot?

>> Source/WebCore/editing/VisiblePosition.cpp:454
>> +    if (highestEditableRootRespectingAXEditing(pos.deepEquivalent()) == highestRoot)
> 
> Ditto.

+1

>> Source/WebCore/editing/VisiblePosition.cpp:471
>> +    Node* highestRoot = highestEditableRootRespectingAXEditing(deepEquivalent());
> 
> Ditto.

+1

>> Source/WebCore/editing/VisiblePosition.cpp:480
>> +    if (highestEditableRootRespectingAXEditing(pos.deepEquivalent()) == highestRoot)
> 
> Ditto.

+1

>> Source/WebCore/editing/VisiblePosition.h:84
>> +    Element* rootEditableElement(EditingBoundaryCrossingRule rule) const { return m_deepPosition.isNotNull() ? m_deepPosition.deprecatedNode()->rootEditableElement(rule) : 0; };
> 
> We should be calling containerNode() instead of deprecatedNode()

And the method above, too?

>> Source/WebCore/editing/htmlediting.cpp:129
>> +        if (nodeIsEditableForRule(node, rule))
> 
> We should probably convert EditingBoundaryCrossingRule to EditableLevel here instead of calling different functions.

rendererIsEditable(EditableLevel) is private, so I'd have to make it public to make that work.

[Ryosuke Niwa]

(In reply to comment #20)
> (From update of attachment 113268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113268&action=review
> 
> >> Source/WebCore/ChangeLog:9
> >> +
> > 
> > You definitely need to explain what changes you're making here. r- due to lack of explanation.
> 
> Will fix once changes are more bedded down.

In that case, please don't set r?. The flag is to be set only if your patch is ready for a formal review.

> >> Source/WebCore/accessibility/AXObjectCache.cpp:662
> >> +    const Element* e = node->isElementNode() ? toElement(node) : node->parentElement();
> > 
> > Please don't use one-letter variable.
> 
> It's a loop variable; if I make the name longer, the for loop below becomes hard to read.

It doesn't really matter. We use one-letter variable only if they're i, j, iter, or their variants. Element should never be abbreviated as stated in the style guideline:
"Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand."

> >> Source/WebCore/dom/Node.cpp:1654
> >> +    return rootEditableElement();
> > 
> > Can this be an inline function?
> 
> I don't know how to judge that, sorry. Do you think it can, as is?

I think so.

> >> Source/WebCore/dom/Node.h:350
> >> +    bool rendererIsAXEditable() const { return rendererIsEditable(AXEditable); }
> > 
> > Again, I'm not digging the function name. How about rendererIsInAriaTextControl?
> 
> As above.

I don't think "AXEditable" is a good name regardless because it doesn't tell us how it's related to accessibility. How about rendererIsEditableAriaRole or rendererIsEditableToAria?

> >> Source/WebCore/editing/VisiblePosition.cpp:445
> >> +    Node* highestRoot = highestEditableRootRespectingAXEditing(deepEquivalent());
> > 
> > I don't think we want these functions to magically change behaviors. Callers of these functions should indicate which editing boundary this function should respect.
> 
> Respecting AX editing boundary only makes sense if a11y is turned on and ARIA editable elements are present, which this method checks. Should I move that check into highestEditableRoot?

The problem is that this change will affect normal editing code as well. When AX is enabled, nextLinePosition and previousLinePosition will start respecting only AX editing boundary instead of editing boundary when called in spell check code, editing code, etc...

This is going to break <div role="textbox">hello<br><span contenteditable>world</span><br>WebKit</div>. With your patch, nextLinePosition and previousLinePosition can get out of the span.

> >> Source/WebCore/editing/htmlediting.cpp:129
> >> +        if (nodeIsEditableForRule(node, rule))
> > 
> > We should probably convert EditingBoundaryCrossingRule to EditableLevel here instead of calling different functions.
> 
> rendererIsEditable(EditableLevel) is private, so I'd have to make it public to make that work.

Okay. That's unfortunate.

[WebKit Review Bot]

Comment on attachment 113819 [details]
Patch

Attachment 113819 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10341019

New failing tests:
editing/pasteboard/paste-delete-insertion-position-skip-paragraph.html
editing/selection/4960116.html

[Alice Boxhall]

Created attachment 114415 [details]
Patch

[Alice Boxhall]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 113268 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=113268&action=review
> > 
> > >> Source/WebCore/ChangeLog:9
> > >> +
> > > 
> > > You definitely need to explain what changes you're making here. r- due to lack of explanation.
> > 
> > Will fix once changes are more bedded down.
> 
> In that case, please don't set r?. The flag is to be set only if your patch is ready for a formal review.

My apologies; I keep forgetting how to get webkit-patch not to set the review bit.

> > >> Source/WebCore/accessibility/AXObjectCache.cpp:662
> > >> +    const Element* e = node->isElementNode() ? toElement(node) : node->parentElement();
> > > 
> > > Please don't use one-letter variable.
> > 
> > It's a loop variable; if I make the name longer, the for loop below becomes hard to read.
> 
> It doesn't really matter. We use one-letter variable only if they're i, j, iter, or their variants. Element should never be abbreviated as stated in the style guideline:
> "Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand."

Ok, done.

> > >> Source/WebCore/dom/Node.cpp:1654
> > >> +    return rootEditableElement();
> > > 
> > > Can this be an inline function?
> > 
> > I don't know how to judge that, sorry. Do you think it can, as is?
> 
> I think so.

Ah -- sadly it can't, as neither Document.h nor AXObjectCache.h are included in Node.h.

> > >> Source/WebCore/dom/Node.h:350
> > >> +    bool rendererIsAXEditable() const { return rendererIsEditable(AXEditable); }
> > > 
> > > Again, I'm not digging the function name. How about rendererIsInAriaTextControl?
> > 
> > As above.
> 
> I don't think "AXEditable" is a good name regardless because it doesn't tell us how it's related to accessibility. How about rendererIsEditableAriaRole or rendererIsEditableToAria?

Hm... ARIA isn't the correct concept here, as ARIA specifically refers to the ARIA spec which allows the web developer to communicate metadata about elements (which would be conveyed to a fully able bodied user via UI cues) to assistive technology, but this method encompasses any element which the accessibility technology should treat as editable, including native editable elements such as textarea etc. Perhaps rendererIsEditableForAX?
 
> > >> Source/WebCore/editing/VisiblePosition.cpp:445
> > >> +    Node* highestRoot = highestEditableRootRespectingAXEditing(deepEquivalent());
> > > 
> > > I don't think we want these functions to magically change behaviors. Callers of these functions should indicate which editing boundary this function should respect.
> > 
> > Respecting AX editing boundary only makes sense if a11y is turned on and ARIA editable elements are present, which this method checks. Should I move that check into highestEditableRoot?
> 
> The problem is that this change will affect normal editing code as well. When AX is enabled, nextLinePosition and previousLinePosition will start respecting only AX editing boundary instead of editing boundary when called in spell check code, editing code, etc...

Not so: as explained above, it is any element which is to be treated as editable by assistive technology, which includes native editable elements. However it does make sense to make sure that _only_ the assistive technology sees the AX editing boundary, so I will fix that, as below.

> This is going to break <div role="textbox">hello<br><span contenteditable>world</span><br>WebKit</div>. With your patch, nextLinePosition and previousLinePosition can get out of the span.

Good point. I will add a test for this and fix the problem.

[Alice Boxhall]

Created attachment 116878 [details]
Patch

[Alice Boxhall]

Not requesting a review as I realise I still need to fix the ChangeLogs and think of some better method names, but I removed the problematic highestEditableRootRespectingAXEditing method and added tests for multiline textboxes.

[Alice Boxhall]

Created attachment 118952 [details]
Patch

[Alice Boxhall]

Still haven't added a long description to the ChangeLog, but I think the code is ready to take another look at now.

[chris fleizach]

Comment on attachment 118952 [details]
Patch

it looks like this patch combines
https://bugs.webkit.org/show_bug.cgi?id=71348

[Alice Boxhall]

(In reply to comment #29)
> (From update of attachment 118952 [details])
> it looks like this patch combines
> https://bugs.webkit.org/show_bug.cgi?id=71348

It does, that's why that bug is listed as a dependency.

[Ryosuke Niwa]

Comment on attachment 118952 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:668
> +const Element* AXObjectCache::rootEditableElementForNode(const Node* node)

You ought to be able to use highestEnclosingNodeOfType here. But maybe that'll be too slow?

> Source/WebCore/accessibility/AXObjectCache.cpp:674
> +    for ( ; element && !element->hasTagName(bodyTag); element = element->parentElement()) {

No space before ;.

> Source/WebCore/accessibility/AXObjectCache.h:112
> +    bool nodeIsEditableToAssistiveTechnology(const Node* node) { return rootEditableElementForNode(node); }

No const?

> Source/WebCore/accessibility/AXObjectCache.h:113
> +    bool nodeIsTextControl(const Node*);

Ditto.

> Source/WebCore/dom/Node.cpp:1648
> +Element* Node::rootEditableElementForAssistiveTechnology() const
> +{
> +    return const_cast<Element*>(document()->axObjectCache()->rootEditableElementForNode(this));
> +}

Why do we need this function? You can just merge it into rootEditableElement.

> Source/WebCore/dom/Node.h:365
> +    bool rendererIsEditable(bool assistiveTechnology = false) const
> +    {
> +        return assistiveTechnology ? rendererIsEditableToAssistiveTechnology(Editable) : rendererIsEditable(Editable);
> +    }
> +
> +    bool rendererIsRichlyEditable(bool assistiveTechnology = false) const
> +    {
> +        return assistiveTechnology ? rendererIsEditableToAssistiveTechnology(RichlyEditable) : rendererIsEditable(RichlyEditable);
> +    }

Why do we need these functions if we're wrapping all calls by nodeIsEditableForRule anyways? In general, we don't want to bloat Node class and rendererIsEditable is extremely hot code (we spend 5-10% of time in here) so we've got to be extremely careful here (of course, inline + constant propagation will probably optimize this code well but I'd like to us confirming it by disassembling the resultant binary if that's not too much to ask.)

> Source/WebCore/editing/htmlediting.cpp:108
> +bool nodeIsEditableForRule(Node* node, EditingBoundaryCrossingRule rule)

This function name isn't great. It sounds as if it calls isContentEditable but we're calling rendererIsEditable. We need to name this function to reflect the fact we can call this function only if the style resolution is up-to-date.

> Source/WebCore/editing/htmlediting.cpp:114
> +    if (rule == CannotCrossEditingBoundary)
> +        return node->rendererIsEditable();
> +    if (rule == CannotCrossAssistiveTechnologyEditingBoundary)
> +        return node->rendererIsEditable(true);
> +    return false;

You should use switch instead so that we can detect this code when a new value is added to the enum in the future.

> Source/WebCore/editing/htmlediting.h:57
> +Node* highestEditableRoot(const Position&, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);

It seems odd for this function to take EditingBoundaryCrossingRule since function is named highest*Editable*Root. e.g. Calling this function with CanCrossEditingBoundary makes no sense.

I think we want to add new enum for our purpose here something like enum EditabilityCriteria { ContentEditable, AXEditable };

[Alice Boxhall]

Created attachment 119156 [details]
Patch

[Alice Boxhall]

Comment on attachment 118952 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:668
>> +const Element* AXObjectCache::rootEditableElementForNode(const Node* node)
> 
> You ought to be able to use highestEnclosingNodeOfType here. But maybe that'll be too slow?

I'm not sure that'll work; highestEnclosingNodeOfType checks rendererIsEditable, which would need to be changed to rendererIsEditable(CannotCrossAssistiveTechnologyEditingBoundary), which ends up calling rootEditableElementForNode as there is no inherited data for AssistiveTechnologyEditing, so we'd get into an infinite loop.

>> Source/WebCore/accessibility/AXObjectCache.cpp:674
>> +    for ( ; element && !element->hasTagName(bodyTag); element = element->parentElement()) {
> 
> No space before ;.

Done.

>> Source/WebCore/accessibility/AXObjectCache.h:112
>> +    bool nodeIsEditableToAssistiveTechnology(const Node* node) { return rootEditableElementForNode(node); }
> 
> No const?

Because it calls nodeIsTextControl()... [continued below]

>> Source/WebCore/dom/Node.h:365
>> +    }
> 
> Why do we need these functions if we're wrapping all calls by nodeIsEditableForRule anyways? In general, we don't want to bloat Node class and rendererIsEditable is extremely hot code (we spend 5-10% of time in here) so we've got to be extremely careful here (of course, inline + constant propagation will probably optimize this code well but I'd like to us confirming it by disassembling the resultant binary if that's not too much to ask.)

I need to be able to distinguish 2x2 situations:
1. Renderer is text editable
2. Renderer is text editable to AT
3. Renderer is richly editable
4. Renderer is richly editable to AT (currently not actually implemented, so it's really just [2]).

These methods already existed, I just made them AT-aware; I wanted to keep rendererIsEditableToAssistiveTechnology(EditableLevel) private, analogous to rendererIsEditable(EditableLevel). So this is how it ended up.

I'm happy to check on the constant propagation thing, but I have no idea how...

>> Source/WebCore/editing/htmlediting.cpp:108
>> +bool nodeIsEditableForRule(Node* node, EditingBoundaryCrossingRule rule)
> 
> This function name isn't great. It sounds as if it calls isContentEditable but we're calling rendererIsEditable. We need to name this function to reflect the fact we can call this function only if the style resolution is up-to-date.

Hm... the existing highestEditableRoot method also checked rendererIsEditable(). Do you have any thoughts on a better name?

>> Source/WebCore/editing/htmlediting.cpp:114
>> +    return false;
> 
> You should use switch instead so that we can detect this code when a new value is added to the enum in the future.

Ok.

>> Source/WebCore/editing/htmlediting.h:57
>> +Node* highestEditableRoot(const Position&, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
> 
> It seems odd for this function to take EditingBoundaryCrossingRule since function is named highest*Editable*Root. e.g. Calling this function with CanCrossEditingBoundary makes no sense.
> 
> I think we want to add new enum for our purpose here something like enum EditabilityCriteria { ContentEditable, AXEditable };

Works for me. Then I can use that in Node::rendererIsEditable() too.

[Alice Boxhall]

Comment on attachment 118952 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.h:113
>> +    bool nodeIsTextControl(const Node*);
> 
> Ditto.

Oops, this comment got lost.

The continuation of the [continued below] was,
... which calls getOrCreate(), which is necessarily non-const. In practice, I don't think AXObjectCache will ever be const; are there any other performance implications of non-const here?

Having thought a lot about it, I think that by the time nodeIsTextControl() or nodeIsEditableToAssistiveTechnology() are called, the AccessibilityObjects in question will either have been created, or need to be created more or less immediately, so it shouldn't be a performance hit. That's because these methods are only called when assistive technology is in use and the given objects are being queried by AT.

Chris Fleizach may well correct me on any of the above.

>> Source/WebCore/dom/Node.cpp:1648
>> +}
> 
> Why do we need this function? You can just merge it into rootEditableElement.

Done.

[chris fleizach]

Comment on attachment 119156 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.h:112
> +    bool nodeIsEditableToAssistiveTechnology(const Node* node) { return rootEditableElementForNode(node); }

we don't use the word "assistive technology" in webkit, we just use "Accessibility"
Why do you need to wrap rootEditableElementForNode() with another method?

> Source/WebCore/dom/Node.cpp:1640
> +        return const_cast<Element*>(document()->axObjectCache()->rootEditableElementForNode(this));

the use of hasARIAEditableElements() doesn't seem like it matters.

[Alice Boxhall]

Comment on attachment 119156 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.h:112
>> +    bool nodeIsEditableToAssistiveTechnology(const Node* node) { return rootEditableElementForNode(node); }
> 
> we don't use the word "assistive technology" in webkit, we just use "Accessibility"
> Why do you need to wrap rootEditableElementForNode() with another method?

Ok, I'll s/AssistiveTechnology/Accessibility/ -- I just find "nodeIsEditableToAccessibility" hard to parse.

The reason it's wrapped is that it's semantically different -- I suppose you could just check if(rootEditableElementForNode(node)) wherever this method is called, this just makes it a bit clearer what's going on.

>> Source/WebCore/dom/Node.cpp:1640
>> +        return const_cast<Element*>(document()->axObjectCache()->rootEditableElementForNode(this));
> 
> the use of hasARIAEditableElements() doesn't seem like it matters.

I don't understand.

It's a performance optimisation; it means it doesn't go into the potentially expensive axObjectCache->rootEditableElementForNode() method if it's going to be equivalent to rootEditableElement() (which it will be if there are no ARIA editable elements).

[chris fleizach]

Comment on attachment 119156 [details]
Patch

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

>>> Source/WebCore/accessibility/AXObjectCache.h:112
>>> +    bool nodeIsEditableToAssistiveTechnology(const Node* node) { return rootEditableElementForNode(node); }
>> 
>> we don't use the word "assistive technology" in webkit, we just use "Accessibility"
>> Why do you need to wrap rootEditableElementForNode() with another method?
> 
> Ok, I'll s/AssistiveTechnology/Accessibility/ -- I just find "nodeIsEditableToAccessibility" hard to parse.
> 
> The reason it's wrapped is that it's semantically different -- I suppose you could just check if(rootEditableElementForNode(node)) wherever this method is called, this just makes it a bit clearer what's going on.

I think that's what is preferred in webkit. it certainly makes things more complicated to have two methods which do the same thing

> Source/WebCore/dom/Node.cpp:821
> +}

it does not seem

>>> Source/WebCore/dom/Node.cpp:1640
>>> +        return const_cast<Element*>(document()->axObjectCache()->rootEditableElementForNode(this));
>> 
>> the use of hasARIAEditableElements() doesn't seem like it matters.
> 
> I don't understand.
> 
> It's a performance optimisation; it means it doesn't go into the potentially expensive axObjectCache->rootEditableElementForNode() method if it's going to be equivalent to rootEditableElement() (which it will be if there are no ARIA editable elements).

so are you saying that when you use aria editable methods the performance will be unacceptable? 
have you characterized how much time is won by using this optimization? 
The trade off of cluttering AXObjectCache doesn't seem like a win to me.

the naming in this block also makes it confusing...
What I see is... the ax object cache has ARIA editable elements, so then we ask for the root element from the ax object cache, which has nothing to do with aria. If I was just looking at it, I would expect that if you're asking for Accessibility root element, then it should always do the accessibility related thing.

[Alice Boxhall]

(In reply to comment #37)
> (From update of attachment 119156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119156&action=review
> 
> >>> Source/WebCore/accessibility/AXObjectCache.h:112
> >>> +    bool nodeIsEditableToAssistiveTechnology(const Node* node) { return rootEditableElementForNode(node); }
> >> 
> >> we don't use the word "assistive technology" in webkit, we just use "Accessibility"
> >> Why do you need to wrap rootEditableElementForNode() with another method?
> > 
> > Ok, I'll s/AssistiveTechnology/Accessibility/ -- I just find "nodeIsEditableToAccessibility" hard to parse.
> > 
> > The reason it's wrapped is that it's semantically different -- I suppose you could just check if(rootEditableElementForNode(node)) wherever this method is called, this just makes it a bit clearer what's going on.
> 
> I think that's what is preferred in webkit. it certainly makes things more complicated to have two methods which do the same thing

Ok.

> > Source/WebCore/dom/Node.cpp:821
> > +}
> 
> it does not seem
> 
> >>> Source/WebCore/dom/Node.cpp:1640
> >>> +        return const_cast<Element*>(document()->axObjectCache()->rootEditableElementForNode(this));
> >> 
> >> the use of hasARIAEditableElements() doesn't seem like it matters.
> > 
> > I don't understand.
> > 
> > It's a performance optimisation; it means it doesn't go into the potentially expensive axObjectCache->rootEditableElementForNode() method if it's going to be equivalent to rootEditableElement() (which it will be if there are no ARIA editable elements).
> 
> so are you saying that when you use aria editable methods the performance will be unacceptable? 
> have you characterized how much time is won by using this optimization? 
> The trade off of cluttering AXObjectCache doesn't seem like a win to me.

You're right -- it probably makes more sense to at least check within the rootEditableElementForNode() method instead.

The performance will depend on how convoluted the DOM is; since this "ARIA editable" concept is not inherited natively the way userModify is, you need to walk up the tree to find out whether there is a parent element with an editable role set on it.

> the naming in this block also makes it confusing...
> What I see is... the ax object cache has ARIA editable elements, so then we ask for the root element from the ax object cache, which has nothing to do with aria. If I was just looking at it, I would expect that if you're asking for Accessibility root element, then it should always do the accessibility related thing.

Makes sense. Will fix that up.

[Alice Boxhall]

Created attachment 119975 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 119975 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.h:111
> +    const Element* rootEditableElementForNode(const Node*);

"ForNode" is redundant. You should probably rename this function to rootAXEditableElement so that the difference with rootEditableElement is clear.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1043
> -        VisiblePosition prevVisiblePos = previousLinePosition(currentVisiblePos, 0);
> +        VisiblePosition prevVisiblePos = previousLinePosition(currentVisiblePos, 0, Accessibility);

I think "Accessibility" is too general. I don't understand what Accessibility in this context mean.

> Source/WebCore/dom/Node.cpp:817
> +    // TODO: respect editableLevel for ARIA editable elements.

Nit: s/TODO/FIXME/; Capitalize respect.

> Source/WebCore/dom/Node.h:361
> +        default:

By using "default" keyword here, we're losing the ability to catch this code at compilation time when a new value is added to the enum.

> Source/WebCore/editing/EditingBoundary.h:40
> +enum EditableTo {
> +    All,
> +    Accessibility
> +};

I don't think "EditableTo" is a descriptive name. Also, we certainly don't want to pollute the WebCore namespace with values like All and Accessibility. We need to be more explicit.
How about
enum AXEditableMode {
    AXEditable,
    ContentEditable,
}
?

[Ryosuke Niwa]

Darin & Enrica, I think you want to look at this patch as well.

[chris fleizach]

Comment on attachment 119975 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:281
> +        m_hasARIAEditableElements = true;

This doesn't look like it's used anywhere (the hasARIAEditableElements() that is)

> Source/WebCore/accessibility/AXObjectCache.cpp:676
> +    const Element* element = node->isElementNode() ? toElement(node) : node->parentElement();

do we ever have cases where the item is not an ElementNode? What if parentNode() was also not an element node?

> Source/WebCore/accessibility/AXObjectCache.cpp:684
> +        return result;

you can set result = rootEditableElement at the top and then you don't need this line

> Source/WebCore/accessibility/AXObjectCache.cpp:694
> +    const AccessibilityObject* axObjectForNode = getOrCreate(node->renderer());

The ForNode is a bit redundant

> Source/WebCore/accessibility/AXObjectCache.cpp:698
> +    return axObjectForNode->isTextControl();

this line can be combined to be 
return axObject && axObject->isTextControl()

[Alice Boxhall]

Comment on attachment 119975 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.h:111
>> +    const Element* rootEditableElementForNode(const Node*);
> 
> "ForNode" is redundant. You should probably rename this function to rootAXEditableElement so that the difference with rootEditableElement is clear.

I thought you didn't like the "AXEditable" naming?

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1043
>> +        VisiblePosition prevVisiblePos = previousLinePosition(currentVisiblePos, 0, Accessibility);
> 
> I think "Accessibility" is too general. I don't understand what Accessibility in this context mean.

I originally had called it "AssistiveTechnology" but cfleizach advised me to rename it to Accessibility to be consistent. I'm not sure what would be a better name.

>> Source/WebCore/dom/Node.cpp:817
>> +    // TODO: respect editableLevel for ARIA editable elements.
> 
> Nit: s/TODO/FIXME/; Capitalize respect.

I thought FIXME indicated that it should be fixed before check-in?

Why capitalise RESPECT, out of interest? I guess that is a standard webkit idiom?

>> Source/WebCore/dom/Node.h:361
>> +        default:
> 
> By using "default" keyword here, we're losing the ability to catch this code at compilation time when a new value is added to the enum.

Ok. I will change it to (whatever we end up calling 'All').

>> Source/WebCore/editing/EditingBoundary.h:40
>> +};
> 
> I don't think "EditableTo" is a descriptive name. Also, we certainly don't want to pollute the WebCore namespace with values like All and Accessibility. We need to be more explicit.
> How about
> enum AXEditableMode {
>     AXEditable,
>     ContentEditable,
> }
> ?

Hm, I definitely agree it's not good to have All and Accessibility in the global namespace. But I think ContentEditable is confusing as well, since that would seem to imply that only contenteditable elements would be used, whereas it's any native editable text including textarea and text input. Maybe "AXEditable" (or "EditableToAX" or "EditableToAccessibility") vs "NativeEditable", and call the enum EditableType or similar?

[chris fleizach]

Comment on attachment 119975 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1043
>>> +        VisiblePosition prevVisiblePos = previousLinePosition(currentVisiblePos, 0, Accessibility);
>> 
>> I think "Accessibility" is too general. I don't understand what Accessibility in this context mean.
> 
> I originally had called it "AssistiveTechnology" but cfleizach advised me to rename it to Accessibility to be consistent. I'm not sure what would be a better name.

AssistiveTechnology and Accessibility are both not great in this context.
The problem is that the enum does not relate to the context. The context is which node to look at for position information.
One looks further up the chain then the other based on some criteria. The enum should capture that essence

[Ryosuke Niwa]

(In reply to comment #43)
> (From update of attachment 119975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119975&action=review
> 
> >> Source/WebCore/accessibility/AXObjectCache.h:111
> >> +    const Element* rootEditableElementForNode(const Node*);
> > 
> > "ForNode" is redundant. You should probably rename this function to rootAXEditableElement so that the difference with rootEditableElement is clear.
> 
> I thought you didn't like the "AXEditable" naming?

Right. I still don't like but AX appears to be the only option given ARIAEditable is incorrect from what you've told me.

> >> Source/WebCore/dom/Node.cpp:817
> >> +    // TODO: respect editableLevel for ARIA editable elements.
> > 
> > Nit: s/TODO/FIXME/; Capitalize respect.
> 
> I thought FIXME indicated that it should be fixed before check-in?
> 
> Why capitalise RESPECT, out of interest? I guess that is a standard webkit idiom?

No, what I mean is "Respect" instead of "respect".

> >> Source/WebCore/editing/EditingBoundary.h:40
> >> +};
> > 
> > I don't think "EditableTo" is a descriptive name. Also, we certainly don't want to pollute the WebCore namespace with values like All and Accessibility. We need to be more explicit.
> > How about
> > enum AXEditableMode {
> >     AXEditable,
> >     ContentEditable,
> > }
> > ?
> 
> Hm, I definitely agree it's not good to have All and Accessibility in the global namespace. But I think ContentEditable is confusing as well, since that would seem to imply that only contenteditable elements would be used, whereas it's any native editable text including textarea and text input.

On my second thought, ContentEditable is a terrible name; it's used everywhere. We need something like ContentIsEditable instead.

> Maybe "AXEditable" (or "EditableToAX" or "EditableToAccessibility") vs "NativeEditable", and call the enum EditableType or similar?

Definitely not "NativeEditable". "Native" in WebKit usually means native to the platform, not to WebCore. How about ContentIsEditable vs. ContentIsAXEditable or ContentIsEditableForAX.

[Alice Boxhall]

Comment on attachment 119975 [details]
Patch

(No code changes yet, just quickly continuing the naming discussion.)

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:281
>> +        m_hasARIAEditableElements = true;
> 
> This doesn't look like it's used anywhere (the hasARIAEditableElements() that is)

Right, I can remove the method as it's now only used within this class.

>> Source/WebCore/accessibility/AXObjectCache.cpp:676
>> +    const Element* element = node->isElementNode() ? toElement(node) : node->parentElement();
> 
> do we ever have cases where the item is not an ElementNode? What if parentNode() was also not an element node?

The Node could be a non-element node in theory, I'm not sure about in practice but I think it's better to check?

parentElement() will always return an Element.

>>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1043
>>>> +        VisiblePosition prevVisiblePos = previousLinePosition(currentVisiblePos, 0, Accessibility);
>>> 
>>> I think "Accessibility" is too general. I don't understand what Accessibility in this context mean.
>> 
>> I originally had called it "AssistiveTechnology" but cfleizach advised me to rename it to Accessibility to be consistent. I'm not sure what would be a better name.
> 
> AssistiveTechnology and Accessibility are both not great in this context.
> The problem is that the enum does not relate to the context. The context is which node to look at for position information.
> One looks further up the chain then the other based on some criteria. The enum should capture that essence

This is a good insight.

How about NativeEditable vs. HasEditableAXRole, or similar?

[chris fleizach]

Comment on attachment 119975 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:674
> +        return rootEditableElement;

I think adding this bool doesn't do much for performance and muddies the code. Running up the parent chain happens constantly. basically it's just following a pointer chain.

[Alice Boxhall]

Comment on attachment 119975 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:674
>> +        return rootEditableElement;
> 
> I think adding this bool doesn't do much for performance and muddies the code. Running up the parent chain happens constantly. basically it's just following a pointer chain.

Not necessarily, if getOrCreate() needs to create a new AccessibilityObject (although I'm pretty sure that won't ever happen in practice).

[chris fleizach]

Comment on attachment 119975 [details]
Patch

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

>>> Source/WebCore/accessibility/AXObjectCache.cpp:674
>>> +        return rootEditableElement;
>> 
>> I think adding this bool doesn't do much for performance and muddies the code. Running up the parent chain happens constantly. basically it's just following a pointer chain.
> 
> Not necessarily, if getOrCreate() needs to create a new AccessibilityObject (although I'm pretty sure that won't ever happen in practice).

That will have happened already if you're in this block of code

[Ryosuke Niwa]

Comment on attachment 119975 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:678
> +    for (; element && !element->hasTagName(bodyTag); element = element->parentElement()) {

It's not okay to stop the search at body. We can have a style rule like "head { display: block; }" with aria roles specified on elements inside head.

[Alice Boxhall]

(In reply to comment #45)
> (In reply to comment #43)
> > (From update of attachment 119975 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=119975&action=review
> > 
> > >> Source/WebCore/accessibility/AXObjectCache.h:111
> > >> +    const Element* rootEditableElementForNode(const Node*);
> > > 
> > > "ForNode" is redundant. You should probably rename this function to rootAXEditableElement so that the difference with rootEditableElement is clear.
> > 
> > I thought you didn't like the "AXEditable" naming?
> 
> Right. I still don't like but AX appears to be the only option given ARIAEditable is incorrect from what you've told me.

Ok, cool.

> > >> Source/WebCore/dom/Node.cpp:817
> > >> +    // TODO: respect editableLevel for ARIA editable elements.
> > > 
> > > Nit: s/TODO/FIXME/; Capitalize respect.
> > 
> > I thought FIXME indicated that it should be fixed before check-in?
> > 
> > Why capitalise RESPECT, out of interest? I guess that is a standard webkit idiom?
> 
> No, what I mean is "Respect" instead of "respect".

Oh. :)

> > >> Source/WebCore/editing/EditingBoundary.h:40
> > Maybe "AXEditable" (or "EditableToAX" or "EditableToAccessibility") vs "NativeEditable", and call the enum EditableType or similar?
> 
> Definitely not "NativeEditable". "Native" in WebKit usually means native to the platform, not to WebCore. How about ContentIsEditable vs. ContentIsAXEditable or ContentIsEditableForAX.

How about
enum EditableType {
    IsEditable,
    HasAXEditableRole
};

?

[Ryosuke Niwa]

Comment on attachment 119975 [details]
Patch

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

> How about
> enum EditableType {
>     IsEditable,
>     HasAXEditableRole
> };
> 
> ?

I'm worried that "IsEditable" is too generic. Maybe EditableWithoutAXRole?

[Alice Boxhall]

(In reply to comment #50)
> (From update of attachment 119975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119975&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:678
> > +    for (; element && !element->hasTagName(bodyTag); element = element->parentElement()) {
> 
> It's not okay to stop the search at body. We can have a style rule like "head { display: block; }" with aria roles specified on elements inside head.

Hm - this is directly copied from Node::rootEditableElement(). Is it incorrect there as well?

Either way, does that mean the search should just stop whenever element is null?

[Ryosuke Niwa]

Comment on attachment 119975 [details]
Patch

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

>>> Source/WebCore/accessibility/AXObjectCache.cpp:678
>>> +    for (; element && !element->hasTagName(bodyTag); element = element->parentElement()) {
>> 
>> It's not okay to stop the search at body. We can have a style rule like "head { display: block; }" with aria roles specified on elements inside head.
> 
> Hm - this is directly copied from Node::rootEditableElement(). Is it incorrect there as well?
> 
> Either way, does that mean the search should just stop whenever element is null?

The check in Node::rootEditableElement is there to ensure we return body element when the document is in the design mode (and we're editing inside body element) since the HTML5 spec says designMode = true is equivalent to setting contentEditable=true on body. But thinking it a little further, we should be explicitly checking that condition there as well. e.g. if the document is not in the design mode and html element has contenteditable=true, then the root editable element should be the html element, not the body element.

[Alice Boxhall]

(In reply to comment #52)
> (From update of attachment 119975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119975&action=review
> 
> > How about
> > enum EditableType {
> >     IsEditable,
> >     HasAXEditableRole
> > };
> > 
> > ?
> 
> I'm worried that "IsEditable" is too generic. Maybe EditableWithoutAXRole?

My initial thought was something like "HasUserModify" but I think that may be too much of an implementation detail. I'd rather not define it in opposition to the HasAXEditableRole option though.

Maybe... RendererIsEditable?

[Alice Boxhall]

(In reply to comment #54)
> (From update of attachment 119975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119975&action=review
> 
> >>> Source/WebCore/accessibility/AXObjectCache.cpp:678
> >>> +    for (; element && !element->hasTagName(bodyTag); element = element->parentElement()) {
> >> 
> >> It's not okay to stop the search at body. We can have a style rule like "head { display: block; }" with aria roles specified on elements inside head.
> > 
> > Hm - this is directly copied from Node::rootEditableElement(). Is it incorrect there as well?
> > 
> > Either way, does that mean the search should just stop whenever element is null?
> 
> The check in Node::rootEditableElement is there to ensure we return body element when the document is in the design mode (and we're editing inside body element) since the HTML5 spec says designMode = true is equivalent to setting contentEditable=true on body. But thinking it a little further, we should be explicitly checking that condition there as well. e.g. if the document is not in the design mode and html element has contenteditable=true, then the root editable element should be the html element, not the body element.

Ok, thanks for explaining that. So, should I make the end condition just element == 0, then?

[Alice Boxhall]

Created attachment 120148 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 120148 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        Reviewed by NOBODY (OOPS!).
> +

This line should appear before the long description.

> Source/WebCore/ChangeLog:25
> +            editable from the point of view of assistive technology, whether natively or otherwise.

We don't normally indent like this.

> Source/WebCore/accessibility/AXObjectCache.cpp:669
> +    const Element* rootEditableElement = node->rootEditableElement();
> +    const Element* result = rootEditableElement;

Why do we need two variables? If nodeIsTextControl(element) is never true, then result is still rootEditableElement.

> Source/WebCore/accessibility/AXObjectCache.cpp:681
> +    if (result)
> +        return result;
> +
> +    return rootEditableElement;

You should be able to always return result here.

> Source/WebCore/accessibility/AXObjectCache.cpp:684
> +bool AXObjectCache::nodeIsTextControl(const Node* node)

We should probably add inline keyword here.

> Source/WebCore/dom/Node.cpp:809
> +bool Node::rendererIsEditableToAccessibility(EditableLevel editableLevel) const

I think rendererIsEditableOrHasEditableAXRole might be better. Since the AX role is set on DOM, not on the renderer.

> Source/WebCore/dom/Node.cpp:1653
> +        return const_cast<Element*>(document()->axObjectCache()->rootAXEditableElement(this));

You should use toElement(Node*) here instead.

> Source/WebCore/editing/EditingBoundary.h:38
> +    RendererIsEditable,

RendererIsEditable is not right. rendererIsEditable exists as supposed to isContentEditable to signify the fact the former doesn't update the layout whereas the latter does. But this enum is nothing to do with that distinction. So It should be renamed to ContentIsEditable.

> LayoutTests/accessibility/textbox-role-reports-line-number.html:14
> +    if (window.layoutTestController && window.accessibilityController) {
> +        window.layoutTestController.dumpAsText();

Do we really need to indent this code?

[chris fleizach]

Comment on attachment 120148 [details]
Patch

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

patch is almost there

> Source/WebCore/accessibility/AXObjectCache.h:110
> +    const Element* rootAXEditableElement(const Node*);

is there a reason you need to make this const Element... I see when you actually use it, you through away the const-ness

> Source/WebCore/editing/EditingBoundary.h:37
> +enum EditableType {

I'm not crazy about these names. 
they don't really communicate one should be the default and the other should be used by accessibility clients
Maybe it should be
DefaultEditability,
AccessibilityRoleEditability

[Ryosuke Niwa]

Comment on attachment 120148 [details]
Patch

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

>> Source/WebCore/editing/EditingBoundary.h:37
>> +enum EditableType {
> 
> I'm not crazy about these names. 
> they don't really communicate one should be the default and the other should be used by accessibility clients
> Maybe it should be
> DefaultEditability,
> AccessibilityRoleEditability

Not sure if that's necessary given we're using one of them as the default argument. If anything, it should signify the fact one is a superset of the other.

[Alice Boxhall]

Created attachment 120261 [details]
Patch

[Alice Boxhall]

Comment on attachment 120148 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +
> 
> This line should appear before the long description.

Done.

>> Source/WebCore/ChangeLog:25
>> +            editable from the point of view of assistive technology, whether natively or otherwise.
> 
> We don't normally indent like this.

Done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:669
>> +    const Element* result = rootEditableElement;
> 
> Why do we need two variables? If nodeIsTextControl(element) is never true, then result is still rootEditableElement.

Done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:681
>> +    return rootEditableElement;
> 
> You should be able to always return result here.

Done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:684
>> +bool AXObjectCache::nodeIsTextControl(const Node* node)
> 
> We should probably add inline keyword here.

Why?

>> Source/WebCore/accessibility/AXObjectCache.h:110
>> +    const Element* rootAXEditableElement(const Node*);
> 
> is there a reason you need to make this const Element... I see when you actually use it, you through away the const-ness

It means it can be called with a const Node* with no need to cast away const anywhere in the method.

It's actually used twice, once where it's interpreted as a bool.

>> Source/WebCore/dom/Node.cpp:809
>> +bool Node::rendererIsEditableToAccessibility(EditableLevel editableLevel) const
> 
> I think rendererIsEditableOrHasEditableAXRole might be better. Since the AX role is set on DOM, not on the renderer.

How about just node::isEditableToAccessibility()?

>> Source/WebCore/dom/Node.cpp:1653
>> +        return const_cast<Element*>(document()->axObjectCache()->rootAXEditableElement(this));
> 
> You should use toElement(Node*) here instead.

This isn't converting from an Node to an Element, but casting away const.

>> Source/WebCore/editing/EditingBoundary.h:37
>> +enum EditableType {
> 
> I'm not crazy about these names. 
> they don't really communicate one should be the default and the other should be used by accessibility clients
> Maybe it should be
> DefaultEditability,
> AccessibilityRoleEditability

I quite like these, actually. Ryosuke, how would you feel about dropping the ContentIsEditable vs. RendererIsEditable decision and going with these instead?

By the way, how do we feel about EditableType? I'm not thrilled about it but hopefully it communicates the right thing.

>> Source/WebCore/editing/EditingBoundary.h:38
>> +    RendererIsEditable,
> 
> RendererIsEditable is not right. rendererIsEditable exists as supposed to isContentEditable to signify the fact the former doesn't update the layout whereas the latter does. But this enum is nothing to do with that distinction. So It should be renamed to ContentIsEditable.

Hm -- it's probably just my lack of in depth understanding, but to me "RendererIsEditable" signifies that that the editability is managed by the renderer, i.e. handled by the browser. "ContentIsEditable" seems to signify only that ... the content is editable, which depending on the context (i.e. whether or not a screen reader is being used), is true in either case.

>> LayoutTests/accessibility/textbox-role-reports-line-number.html:14
>> +        window.layoutTestController.dumpAsText();
> 
> Do we really need to indent this code?

Yes? Why wouldn't it be indented?

[WebKit Review Bot]

Comment on attachment 120261 [details]
Patch

Attachment 120261 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11000322

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Ryosuke Niwa]

Comment on attachment 120261 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:1653
> +        return const_cast<Element *>(document()->axObjectCache()->rootAXEditableElement(this));

Nit: No space between Element and *.

> Source/WebCore/dom/Node.h:359
> +        case RendererIsEditable:

I don't think "RendererIsEditable" accurate. Again, the reason this method is called "rendererIsEditable" is because we had to signify the fact we're not calling updateLayout. However, the concept of whether node is editable or not is independent of this concept. For this reason, I'd prefer calling it "ContentIsEditable".

> Source/WebCore/editing/visible_units.cpp:659
> +    // TODO: pull out code that is common between this and previousLinePosition.

s/TODO/FIXME/. Also this FIXME is wrong. Despite of what it looks like, there aren't that much code duplication between the two functions. They call different functions next/prev, left/right.

> LayoutTests/accessibility/textbox-role-reports-line-number.html:1
> +<!DOCTYPE HTML PUBLIC>

Why public?

> LayoutTests/accessibility/textbox-role-reports-line-number.html:5
> +<head>
> +<script src="../fast/js/resources/js-test-pre.js"></script>
> +</head>

No need to have a head. You can put the script element in the body.

> LayoutTests/accessibility/textbox-role-reports-line-number.html:13
> +    if (window.layoutTestController && window.accessibilityController) {
> +        window.layoutTestController.dumpAsText();

No need to indent the entire script like this.

[Alice Boxhall]

Created attachment 121014 [details]
Patch

[Alice Boxhall]

Comment on attachment 120261 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:1653
>> +        return const_cast<Element *>(document()->axObjectCache()->rootAXEditableElement(this));
> 
> Nit: No space between Element and *.

Done.

>> Source/WebCore/dom/Node.h:359
>> +        case RendererIsEditable:
> 
> I don't think "RendererIsEditable" accurate. Again, the reason this method is called "rendererIsEditable" is because we had to signify the fact we're not calling updateLayout. However, the concept of whether node is editable or not is independent of this concept. For this reason, I'd prefer calling it "ContentIsEditable".

Ok, I think ContentIsEditable makes sense in context, although I still find it a little ambiguous. I think it makes the most sense, though.

>> Source/WebCore/editing/visible_units.cpp:659
>> +    // TODO: pull out code that is common between this and previousLinePosition.
> 
> s/TODO/FIXME/. Also this FIXME is wrong. Despite of what it looks like, there aren't that much code duplication between the two functions. They call different functions next/prev, left/right.

I count 7 significant differences between the two functions, out of 51 LOC (not counting comments or newlines). One difference seems like it shouldn't be a difference:

-                ASSERT(n->renderer());

... is in nextLinePosition but not previousLinePosition. Each other difference could just as easily be based on a boolean switch, except for maybe

-            Position pos = createLegacyEditingPosition(n, caretMinOffset(n));
+            Position pos = n->hasTagName(brTag) ? positionBeforeNode(n) : createLegacyEditingPosition(n, caretMaxOffset(n));

The reason I added the TODO (FIXME) is that I was having a lot of trouble making sure my changes two the two methods matched up. I am happy to remove it though, if you prefer.

>> LayoutTests/accessibility/textbox-role-reports-line-number.html:1
>> +<!DOCTYPE HTML PUBLIC>
> 
> Why public?

No reason.

>> LayoutTests/accessibility/textbox-role-reports-line-number.html:5
>> +</head>
> 
> No need to have a head. You can put the script element in the body.

Done.

>> LayoutTests/accessibility/textbox-role-reports-line-number.html:13
>> +        window.layoutTestController.dumpAsText();
> 
> No need to indent the entire script like this.

Got it -- I didn't know what you were referring to previously, sorry!

[Ryosuke Niwa]

Comment on attachment 120261 [details]
Patch

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

>>> Source/WebCore/editing/visible_units.cpp:659
>>> +    // TODO: pull out code that is common between this and previousLinePosition.
>> 
>> s/TODO/FIXME/. Also this FIXME is wrong. Despite of what it looks like, there aren't that much code duplication between the two functions. They call different functions next/prev, left/right.
> 
> I count 7 significant differences between the two functions, out of 51 LOC (not counting comments or newlines). One difference seems like it shouldn't be a difference:
> 
> -                ASSERT(n->renderer());
> 
> ... is in nextLinePosition but not previousLinePosition. Each other difference could just as easily be based on a boolean switch, except for maybe
> 
> -            Position pos = createLegacyEditingPosition(n, caretMinOffset(n));
> +            Position pos = n->hasTagName(brTag) ? positionBeforeNode(n) : createLegacyEditingPosition(n, caretMaxOffset(n));
> 
> The reason I added the TODO (FIXME) is that I was having a lot of trouble making sure my changes two the two methods matched up. I am happy to remove it though, if you prefer.

We don't normally do those boolean checks per convention so I'd prefer not having that FIXME.

[Gyuyoung Kim]

Comment on attachment 121014 [details]
Patch

Attachment 121014 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11079191

[Alice Boxhall]

Created attachment 121019 [details]
Patch

[Alice Boxhall]

(In reply to comment #68)
> (From update of attachment 121014 [details])
> Attachment 121014 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/11079191

Strange, how did that work every other time? The code hasn't changed significantly.

[Alice Boxhall]

Comment on attachment 120261 [details]
Patch

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

>>>> Source/WebCore/editing/visible_units.cpp:659
>>>> +    // TODO: pull out code that is common between this and previousLinePosition.
>>> 
>>> s/TODO/FIXME/. Also this FIXME is wrong. Despite of what it looks like, there aren't that much code duplication between the two functions. They call different functions next/prev, left/right.
>> 
>> I count 7 significant differences between the two functions, out of 51 LOC (not counting comments or newlines). One difference seems like it shouldn't be a difference:
>> 
>> -                ASSERT(n->renderer());
>> 
>> ... is in nextLinePosition but not previousLinePosition. Each other difference could just as easily be based on a boolean switch, except for maybe
>> 
>> -            Position pos = createLegacyEditingPosition(n, caretMinOffset(n));
>> +            Position pos = n->hasTagName(brTag) ? positionBeforeNode(n) : createLegacyEditingPosition(n, caretMaxOffset(n));
>> 
>> The reason I added the TODO (FIXME) is that I was having a lot of trouble making sure my changes two the two methods matched up. I am happy to remove it though, if you prefer.
> 
> We don't normally do those boolean checks per convention so I'd prefer not having that FIXME.

Ok, removed.

[Alice Boxhall]

Chris, would you be able to take another look at this now?

[chris fleizach]

Comment on attachment 121019 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:671
> +    for (; element && !element->hasTagName(bodyTag); element = element->parentElement()) {

should this just check against element == 0 and run all the way up the parent chain?

> Source/WebCore/dom/Node.cpp:802
> +    ASSERT(document()->axObjectCacheExists());

are these asserts necessary? this code will crash in production if any of these assertions are not met so having the asserts won't do much (except for axEnabled that is)

> Source/WebCore/dom/Node.h:381
> +

rendererIsRichlyEditable and rendererIsEditable are basically the same. can they be combined in a way or at least both of them call a common method that takes one more argument

> Source/WebCore/editing/visible_units.cpp:642
> +static Node* nextLeafWithSameEditability(Node* node, EditableType editableType = HasEditableAXRole)

why is the default here the AX case?

[Alice Boxhall]

Created attachment 121177 [details]
Patch

[Alice Boxhall]

Created attachment 121179 [details]
Patch

[Alice Boxhall]

Comment on attachment 121019 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:671
>> +    for (; element && !element->hasTagName(bodyTag); element = element->parentElement()) {
> 
> should this just check against element == 0 and run all the way up the parent chain?

Ah yes, there was some discussion with Ryosuke on this earlier, wasn't there. I think that sounds like the right thing to do.

>> Source/WebCore/dom/Node.cpp:802
>> +    ASSERT(document()->axObjectCacheExists());
> 
> are these asserts necessary? this code will crash in production if any of these assertions are not met so having the asserts won't do much (except for axEnabled that is)

I don't follow why the accessibilityEnabled assert will help where the others won't, sorry.

Regardless, I'm not sure whether the asserts are necessary, but it seems to be the convention in this class to use asserts like this. Ryosuke may know more.

>> Source/WebCore/dom/Node.h:381
>> +
> 
> rendererIsRichlyEditable and rendererIsEditable are basically the same. can they be combined in a way or at least both of them call a common method that takes one more argument

These are pre-existing methods which are called throughout the codebase; I think that change is outside the scope of this patch.

>> Source/WebCore/editing/visible_units.cpp:642
>> +static Node* nextLeafWithSameEditability(Node* node, EditableType editableType = HasEditableAXRole)
> 
> why is the default here the AX case?

Oh, good question, that seems very wrong. Fixed.

[Ryosuke Niwa]

Comment on attachment 121177 [details]
Patch

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

> Source/WebCore/editing/visible_units.h:58
> +VisiblePosition previousLinePosition(const VisiblePosition &, int lineDirectionPoint, EditableType = ContentIsEditable);
> +VisiblePosition nextLinePosition(const VisiblePosition &, int lineDirectionPoint, EditableType = ContentIsEditable);

Could you remove the space between VisiblePosition and &?

> LayoutTests/accessibility/textbox-role-reports-line-number.html:19
> +             + " axElement.insertionPointLineNumber", "0");

Wrong indentation. + should be exactly 4 spaces to the right of s in shouldBe.

> LayoutTests/accessibility/textbox-role-reports-line-number.html:29
> +             + " axElement.insertionPointLineNumber", "0");
> +    shouldBe("window.getSelection().setBaseAndExtent(multilineAriaTextBox.childNodes[1], 1, multilineAriaTextBox.childNodes[1], 1);"
> +             + " axElement.insertionPointLineNumber", "1");

Ditto.

[Ryosuke Niwa]

Comment on attachment 121179 [details]
Patch

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

r=me provided nits are addressed.

> Source/WebCore/ChangeLog:9
> +        set by the developer on an element which behaves like an editable text element (such as a

s/developer/author/; also "set on an element" seems sufficient here.

> Source/WebCore/ChangeLog:15
> +        visible_units, aware of the notion that an element may be non-natively editable, via the
> +        EditableType enum added to EditingBoundary.h.

the word "native" usually refers to platform-naiveness. Also, saying non-native doesn't tell us what they're so it's probably better to say "an element maybe editable via text-control ARIA role"

[chris fleizach]

(In reply to comment #78)
> (From update of attachment 121179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121179&action=review
> 
> r=me provided nits are addressed.
> 
> > Source/WebCore/ChangeLog:9
> > +        set by the developer on an element which behaves like an editable text element (such as a
> 
> s/developer/author/; also "set on an element" seems sufficient here.
> 
> > Source/WebCore/ChangeLog:15
> > +        visible_units, aware of the notion that an element may be non-natively editable, via the
> > +        EditableType enum added to EditingBoundary.h.
> 
> the word "native" usually refers to platform-naiveness. Also, saying non-native doesn't tell us what they're so it's probably better to say "an element maybe editable via text-control ARIA role"

Assert issue needs to be addressed before checking in. 

ASSERTS are meant to catch logically wrong cases in debug, but should presumably fail gracefully in production.

In this patch, both debug and production will crash if those conditions are met

[Alice Boxhall]

Created attachment 121184 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 121184 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        input field or contenteditable text), but whose behaviour is controlled by the developer

s/developer/author/

[Alice Boxhall]

(In reply to comment #79)
> (In reply to comment #78)
> > (From update of attachment 121179 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121179&action=review
> > 
> > r=me provided nits are addressed.
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        set by the developer on an element which behaves like an editable text element (such as a
> > 
> > s/developer/author/; also "set on an element" seems sufficient here.
> > 
> > > Source/WebCore/ChangeLog:15
> > > +        visible_units, aware of the notion that an element may be non-natively editable, via the
> > > +        EditableType enum added to EditingBoundary.h.
> > 
> > the word "native" usually refers to platform-naiveness. Also, saying non-native doesn't tell us what they're so it's probably better to say "an element maybe editable via text-control ARIA role"
> 
> Assert issue needs to be addressed before checking in. 
> 
> ASSERTS are meant to catch logically wrong cases in debug, but should presumably fail gracefully in production.
> 
> In this patch, both debug and production will crash if those conditions are met

There are many cases in this class where that is true, although that doesn't mean that this is right.

Should I check those cases and return false if those conditions are not met, then?

[chris fleizach]

(In reply to comment #82)
> (In reply to comment #79)
> > (In reply to comment #78)
> > > (From update of attachment 121179 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=121179&action=review
> > > 
> > > r=me provided nits are addressed.
> > > 
> > > > Source/WebCore/ChangeLog:9
> > > > +        set by the developer on an element which behaves like an editable text element (such as a
> > > 
> > > s/developer/author/; also "set on an element" seems sufficient here.
> > > 
> > > > Source/WebCore/ChangeLog:15
> > > > +        visible_units, aware of the notion that an element may be non-natively editable, via the
> > > > +        EditableType enum added to EditingBoundary.h.
> > > 
> > > the word "native" usually refers to platform-naiveness. Also, saying non-native doesn't tell us what they're so it's probably better to say "an element maybe editable via text-control ARIA role"
> > 
> > Assert issue needs to be addressed before checking in. 
> > 
> > ASSERTS are meant to catch logically wrong cases in debug, but should presumably fail gracefully in production.
> > 
> > In this patch, both debug and production will crash if those conditions are met
> 
> There are many cases in this class where that is true, although that doesn't mean that this is right.
> 
> Should I check those cases and return false if those conditions are not met, then?

I would ASSERT all three
but also check that document() and axObjectCacheExists(), so that we won't crash in production if those asserts are for some reason not met. thanks

[Alice Boxhall]

Created attachment 121191 [details]
Patch

[Alice Boxhall]

Created attachment 121206 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121206 [details]
Patch for landing

Rejecting attachment 121206 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

[e-mail] found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11110376

[Ryosuke Niwa]

Comment on attachment 121206 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by [e-mail].

Why is it filled by my email address? Did webkit-patch regress?

[Alice Boxhall]

(In reply to comment #87)
> (From update of attachment 121206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121206&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by [e-mail].
> 
> Why is it filled by my email address? Did webkit-patch regress?

My fault: webkit-patch didn't find a reviewer, and I used --reviewer incorrectly.

How should I specify you as the reviewer?

[Alice Boxhall]

Created attachment 121364 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121364 [details]
Patch for landing

Rejecting attachment 121364 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

[e-mail] found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11145189

[Alice Boxhall]

Created attachment 121394 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121394 [details]
Patch for landing

Clearing flags on attachment: 121394

Committed r104276: <http://trac.webkit.org/changeset/104276>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.