Added ability to distinguished between rich and plain text editables on accessibility objects.

Added ability to distinguished between rich and plain text editables on accessibility objects.

BUG=491027
R=dmazzoni,pfeldman

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196783

[dmazzoni, 2015-05-28 05:55:56]

https://codereview.chromium.org/1156353003/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1156353003/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXObject.cpp:756: if (node->hasEditableStyle())
This will return true for everything in the contenteditable subtree, is that
what you want?

Also, the spec allows contentEditable="plaintext-only" (see
http://w3c.github.io/editing-explainer/contentEditable.html), but in that case
this function would still return true, is that what you want?

Maybe Node::isContentRichlyEditable is better, but maybe add a test for that.

[blink-reviews, 2015-05-28 06:03:42]

On 5/28/2015 1:55 AM, dmazzoni@chromium.org wrote:
>
>
https://codereview.chromium.org/1156353003/diff/1/Source/modules/accessibilit...

>
> File Source/modules/accessibility/AXObject.cpp (right):
>
>
https://codereview.chromium.org/1156353003/diff/1/Source/modules/accessibilit...

>
> Source/modules/accessibility/AXObject.cpp:756: if
> (node->hasEditableStyle())
> This will return true for everything in the contenteditable subtree, is
> that what you want?

Currently I only use this on the Chromium side for content editables. 
However, I deliberately set this to true for all the descendants because 
it reflects their true state. Since contenteditable affects all its 
descendants, it would be a lie to say that the descendants are not 
richly editable.

I'll fix the other things you pointed out tomorrow.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[blink-reviews, 2015-06-04 01:09:41]

Comments addressed.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-06-04 03:42:28]

For the record: the corresponding test is here (in case any drive-by code
reviewers want to see it)

https://codereview.chromium.org/1149293009/

[dmazzoni, 2015-06-04 03:45:26]

Remember that inside a canvas or for a node that's visible only to screen
readers using aria-hidden=false, you'll have an AXNodeObject and node an
AXLayoutObject

Please implement this on AXNodeObject to check the node, and have AXLayoutObject
call the inherited implementation and then check the WebArea case.

https://codereview.chromium.org/1156353003/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXLayoutObject.h (right):

https://codereview.chromium.org/1156353003/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXLayoutObject.h:88: virtual bool
isContentRichlyEditable() const override;
As I said in the other change, maybe just isRichlyEditable, but I don't have
strong feelings

[blink-reviews, 2015-06-04 16:19:18]

I thought of that but when I read the code for 
isContentRichlyEditable(), I found out that it relies on checking for 
the -webkit-user-modify style, which requires a layout object to be 
present. That's why in my layout test I added a test case for 
-webkit-user-modify: read-only.
The code goes like this:
if (isPseudoElement) return false;
else look through the tree starting from the current node and traveling 
up its parents and only test layout objects.
Otherwise return false.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-06-04 16:37:25]

lgtm

OK, that makes sense - if we can't really have rich text in AXNodeObject that's
fine, maybe just add a comment to that effect so that somebody doesn't try to
refactor this into AXNodeObject in the future.

[blink-reviews, 2015-06-04 18:36:44]

Your comments have been addressed.
Added a comment in the isRichlyEditable method for devs not to move 
node() logic to AXObjectNode.
Added TODO for me to figure out canvas and aria-hidden="false".

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[nektarios, 2015-06-04 23:59:07]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-06-04 23:59:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1156353003/#ps40001
(title: "Renamed to isRichlyEditable.")

[commit-bot: I haz the power, 2015-06-04 23:59:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156353003/40001

[commit-bot: I haz the power, 2015-06-05 00:05:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 00:06:00]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[nektarios, 2015-06-05 00:12:56]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2015-06-05 00:13:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156353003/40001

[commit-bot: I haz the power, 2015-06-05 00:18:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 00:18:50]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[nektarios, 2015-06-05 00:29:31]

nektar@chromium.org changed reviewers:
+ pfeldman@chromium.org

[nektarios, 2015-06-05 00:32:39]

On 2015/06/05 at 00:18:50, commit-bot wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

+pfeldman
Please approve my an additional public accessibility API in web/WebAXObject.h.

[dglazkov, 2015-06-09 15:45:56]

dglazkov@chromium.org changed reviewers:
+ dglazkov@chromium.org

[dglazkov, 2015-06-09 15:45:57]

lgtm

[nektarios, 2015-06-09 15:47:42]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2015-06-09 15:48:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156353003/40001

[commit-bot: I haz the power, 2015-06-09 16:54:54]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196783