More precise use of multiline state

More precise use of multiline state. It only needs to be exposed at the root of the editable area. ARIA should override native markup. Add lots of tests.

BUG=730138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2931893002
Cr-Commit-Position: refs/heads/master@{#485051}
Committed: https://chromium.googlesource.com/chromium/src/+/5a1e7c0f36fc682b8572f3d781943aef6edce818

[aleventhal, 2017-06-08 17:04:42]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 17:05:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 17:38:09]

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

[commit-bot: I haz the power, 2017-06-08 17:38:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleventhal, 2017-06-09 15:34:03]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 15:34:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 16:36:17]

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

[commit-bot: I haz the power, 2017-06-09 16:36:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aleventhal, 2017-06-09 16:53:52]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 18:07:24]

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

[commit-bot: I haz the power, 2017-06-09 18:07:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aleventhal, 2017-06-09 19:33:39]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 19:34:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-09 19:34:32]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 19:35:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 20:12:08]

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

[commit-bot: I haz the power, 2017-06-09 20:12:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleventhal, 2017-06-09 20:45:46]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 20:46:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 21:16:37]

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

[commit-bot: I haz the power, 2017-06-09 21:16:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleventhal, 2017-06-09 21:50:47]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 21:51:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 22:29:11]

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

[commit-bot: I haz the power, 2017-06-09 22:29:13]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleventhal, 2017-06-12 13:56:37]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 13:56:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 14:46:40]

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

[commit-bot: I haz the power, 2017-06-12 14:46:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleventhal, 2017-06-12 16:27:05]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 16:27:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 17:36:32]

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

[commit-bot: I haz the power, 2017-06-12 17:36:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aleventhal, 2017-06-12 18:26:45]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 18:27:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 19:14:51]

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

[commit-bot: I haz the power, 2017-06-12 19:14:52]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[aleventhal, 2017-06-21 04:40:19]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 04:40:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 04:43:12]

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

[commit-bot: I haz the power, 2017-06-21 04:43:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[aleventhal, 2017-06-21 15:07:52]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 15:08:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 15:28:26]

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

[commit-bot: I haz the power, 2017-06-21 15:28:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[aleventhal, 2017-06-21 15:57:50]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 15:58:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 16:19:59]

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

[commit-bot: I haz the power, 2017-06-21 16:20:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aleventhal, 2017-06-21 17:01:03]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 17:01:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-21 17:27:08]

Description was changed from

==========
More precise use of multiline state

BUG=730138
==========

to

==========
More precise use of multiline state

BUG=730138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-06-21 17:27:30]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 17:27:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 18:07:37]

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

[commit-bot: I haz the power, 2017-06-21 18:07:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[aleventhal, 2017-06-23 17:30:39]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 17:30:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-23 17:41:27]

Description was changed from

==========
More precise use of multiline state

BUG=730138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
More precise use of multiline state. It only needs to be exposed at the root of
the editable area. ARIA should override native markup. Add lots of tests.

BUG=730138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aleventhal, 2017-06-23 17:41:27]

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

[aleventhal, 2017-06-23 17:41:44]

Ready for review.

[commit-bot: I haz the power, 2017-06-23 18:21:24]

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

[commit-bot: I haz the power, 2017-06-23 18:21:25]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dougt, 2017-06-23 18:27:16]

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

[dougt, 2017-06-23 18:27:18]

Note that this will conflict with my change and not sure who's going first.

https://codereview.chromium.org/2943243002

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:4315:
BrowserAccessibility* parent = owner()->PlatformGetParent();
I am confused by this. 

GetParent() is just returning the node above |this| one.  Shouldn't we walk up
the tree to the root?

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:4385: // direct
text-only children should not be present at tree  s and so no
I don't think you want this change.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:5103: }
Since this is a one liner, you don't need the curly braces.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
File content/browser/accessibility/browser_accessibility_com_win.h (right):

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.h:817: // Is the
root of an editable area
Nit: Comment isn't needed.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.h:820: // Is an
editbox of some type
Same.

[aleventhal, 2017-06-24 18:05:50]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 18:05:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-24 18:08:58]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 18:09:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 20:17:41]

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

[commit-bot: I haz the power, 2017-06-24 20:17:42]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aleventhal, 2017-06-24 22:01:34]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 22:01:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-25 00:05:38]

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

[commit-bot: I haz the power, 2017-06-25 00:05:39]

Dry run: This issue passed the CQ dry run.

[nektarios, 2017-06-26 15:57:55]

+    if (breaks.length < 1)
+      return 0;

Isn't this the same as saying
if (!breaks.length)


+bool BrowserAccessibilityComWin::IsDocument() const
Better move this to BrowserAccessibility or better AXPlatformNode. I can see me
using it on Mac too and perhaps it can also be used on Android?

+bool BrowserAccessibilityComWin::IsRootEditable() const
+bool BrowserAccessibilityComWin::IsEditBoxRole() const
I think there are some functions in BrowserAccessibility that already do this.
Look at
BrowserAccessibility::IsSimpleTextControl and IsRichTextControl.
And modify them if necessary.

Also, how about combo box role? Shouldn't that be considered a text field.

+  if (is_editable ? IsRootEditable() : IsEditBoxRole()) {
Doesn't IsRootEditable imply is_editable?
Why not simply:
if (IsRootEditable() || IsEditBoxRole())

+    // Support multi/single line states if root edtiable or appropriate role.
Root editable has a spelling mistake.


+++ b/content/test/data/accessibility/aria/aria-textbox-with-selection.html
+  <!-- Will not expose multiline state because it's not actually editable -->
Add a period at the end of the comment.
Does the file miss a DOCTYPE?
<!DOCTYPE html>


AXNodeObject.h
+  bool IsMultiline() const;
Don't you need to say override at the end of this declaration?

AXNodeObject.h
AXNodeObject::IsMultiline
Put a period at the end of all your comments.

Could you make use of the already existing method
bool AXNodeObject::IsTextControl() const
Or modify that one if it doesn't suit your needs.

+  if (is_edit_box)
+    return false;
Is this actually correct?
How about:
<div contentEditable role="textbox">
Shouldn't this be multiline by default?


/content/test/data/accessibility/html/input-text-value-expected-win.txt
b/content/test/data/accessibility/html/input-text-value-expected-win.txt
new file mode 100644
Do you know why it talks about a new file mode?

[aleventhal, 2017-06-26 18:44:39]

On 2017/06/26 15:57:55, nektarios wrote:
> +    if (breaks.length < 1)
> +      return 0;
> 
> Isn't this the same as saying
> if (!breaks.length)

It's the same. I'll change it.

> +bool BrowserAccessibilityComWin::IsDocument() const
> Better move this to BrowserAccessibility or better AXPlatformNode. I can see
me
> using it on Mac too and perhaps it can also be used on Android?
I moved it to BrowserAccessibility.

> 
> +bool BrowserAccessibilityComWin::IsRootEditable() const
> +bool BrowserAccessibilityComWin::IsEditBoxRole() const
> I think there are some functions in BrowserAccessibility that already do this.
> Look at
> BrowserAccessibility::IsSimpleTextControl and IsRichTextControl.
> And modify them if necessary.
Awesome suggestion on IsRichTextControl().
I can't use IsSimpleTextControl() too easily because it includes combo boxes,
and it doesn't make sense to expose single/multi line states on those.

> Also, how about combo box role? Shouldn't that be considered a text field.
I don't expect it to be single or multiline. Also you don't generally have a
caret in there. The multiline state is really to give the user a hint about what
happens if they hit down arrow or Enter in an editable area.
 
> +  if (is_editable ? IsRootEditable() : IsEditBoxRole()) {
> Doesn't IsRootEditable imply is_editable?
> Why not simply:
> if (IsRootEditable() || IsEditBoxRole())
I'll try that.

> 
> +    // Support multi/single line states if root edtiable or appropriate role.
> Root editable has a spelling mistake.
> 
Thanks.

> 
> +++ b/content/test/data/accessibility/aria/aria-textbox-with-selection.html
> +  <!-- Will not expose multiline state because it's not actually editable -->
> Add a period at the end of the comment.
> Does the file miss a DOCTYPE?
> <!DOCTYPE html>
I don't see any missing doctype. Which file?
> 
> 
> AXNodeObject.h
> +  bool IsMultiline() const;
> Don't you need to say override at the end of this declaration?
> 
> AXNodeObject.h
> AXNodeObject::IsMultiline
> Put a period at the end of all your comments.
Ok.

> 
> Could you make use of the already existing method
> bool AXNodeObject::IsTextControl() const
> Or modify that one if it doesn't suit your needs.
I don't see how to modify it without making things more complicated. This seems
simpler to me.

> 
> +  if (is_edit_box)
> +    return false;
> Is this actually correct?
> How about:
> <div contentEditable role="textbox">
> Shouldn't this be multiline by default?
I would have guessed that too, but according to CORE-AAM, aria-multiline is
false by default.

> 
> 
> /content/test/data/accessibility/html/input-text-value-expected-win.txt
> b/content/test/data/accessibility/html/input-text-value-expected-win.txt
> new file mode 100644
> Do you know why it talks about a new file mode?
I think it's just saying there is a new file, and that is the mode. That file
didn't exist before.

[aleventhal, 2017-06-26 18:59:52]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 19:00:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blink-reviews, 2017-06-26 19:16:42]

AXNodeObject.h
> > + bool IsMultiline() const;

Don't you need to say override at the end of this declaration?



 > +bool BrowserAccessibilityComWin::IsRootEditable() const
> > +bool BrowserAccessibilityComWin::IsEditBoxRole() const
> > I think there are some functions in BrowserAccessibility that 
> already do this.
> > Look at
> > BrowserAccessibility::IsSimpleTextControl and IsRichTextControl.
> > And modify them if necessary.
> Awesome suggestion on IsRichTextControl().
> I can't use IsSimpleTextControl() too easily because it includes combo 
> boxes,
> and it doesn't make sense to expose single/multi line states on those.
>
> > Also, how about combo box role? Shouldn't that be considered a text 
> field.
> I don't expect it to be single or multiline. Also you don't generally 
> have a
> caret in there. The multiline state is really to give the user a hint 
> about what
> happens if they hit down arrow or Enter in an editable area.
>
So the following multiline attribute should be ignored?
<input role="combobox" aria-multiline="true">


How about you call your helper method IsTextOrSearchField to make it 
clear that we are excluding combo box on purpose?



> >
> > Could you make use of the already existing method
> > bool AXNodeObject::IsTextControl() const
> > Or modify that one if it doesn't suit your needs.
> I don't see how to modify it without making things more complicated. 
> This seems
> simpler to me.
>
Okay, but could we call the variable IsTextOrSearchField or something to 
make it clear that we are excluding combo boxes on purpose?

> >
> > + if (is_edit_box)
> > + return false;
> > Is this actually correct?
> > How about:
> > <div contentEditable role="textbox">
> > Shouldn't this be multiline by default?
> I would have guessed that too, but according to CORE-AAM, 
> aria-multiline is
> false by default.

Can we put a comment explaining that, because I or someone else might 
forget and change it back in the future?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-26 19:16:42]

AXNodeObject.h
> > + bool IsMultiline() const;

Don't you need to say override at the end of this declaration?



 > +bool BrowserAccessibilityComWin::IsRootEditable() const
> > +bool BrowserAccessibilityComWin::IsEditBoxRole() const
> > I think there are some functions in BrowserAccessibility that 
> already do this.
> > Look at
> > BrowserAccessibility::IsSimpleTextControl and IsRichTextControl.
> > And modify them if necessary.
> Awesome suggestion on IsRichTextControl().
> I can't use IsSimpleTextControl() too easily because it includes combo 
> boxes,
> and it doesn't make sense to expose single/multi line states on those.
>
> > Also, how about combo box role? Shouldn't that be considered a text 
> field.
> I don't expect it to be single or multiline. Also you don't generally 
> have a
> caret in there. The multiline state is really to give the user a hint 
> about what
> happens if they hit down arrow or Enter in an editable area.
>
So the following multiline attribute should be ignored?
<input role="combobox" aria-multiline="true">


How about you call your helper method IsTextOrSearchField to make it 
clear that we are excluding combo box on purpose?



> >
> > Could you make use of the already existing method
> > bool AXNodeObject::IsTextControl() const
> > Or modify that one if it doesn't suit your needs.
> I don't see how to modify it without making things more complicated. 
> This seems
> simpler to me.
>
Okay, but could we call the variable IsTextOrSearchField or something to 
make it clear that we are excluding combo boxes on purpose?

> >
> > + if (is_edit_box)
> > + return false;
> > Is this actually correct?
> > How about:
> > <div contentEditable role="textbox">
> > Shouldn't this be multiline by default?
> I would have guessed that too, but according to CORE-AAM, 
> aria-multiline is
> false by default.

Can we put a comment explaining that, because I or someone else might 
forget and change it back in the future?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aleventhal, 2017-06-26 19:28:49]

On 2017/06/26 19:16:42, chromium-reviews wrote:
> AXNodeObject.h
> > > + bool IsMultiline() const;
> 
> Don't you need to say override at the end of this declaration?
Yes, I've added that.
> 
> 
> 
>  > +bool BrowserAccessibilityComWin::IsRootEditable() const
> > > +bool BrowserAccessibilityComWin::IsEditBoxRole() const
> > > I think there are some functions in BrowserAccessibility that 
> > already do this.
> > > Look at
> > > BrowserAccessibility::IsSimpleTextControl and IsRichTextControl.
> > > And modify them if necessary.
> > Awesome suggestion on IsRichTextControl().
> > I can't use IsSimpleTextControl() too easily because it includes combo 
> > boxes,
> > and it doesn't make sense to expose single/multi line states on those.
> >
> > > Also, how about combo box role? Shouldn't that be considered a text 
> > field.
> > I don't expect it to be single or multiline. Also you don't generally 
> > have a
> > caret in there. The multiline state is really to give the user a hint 
> > about what
> > happens if they hit down arrow or Enter in an editable area.
> >
> So the following multiline attribute should be ignored?
> <input role="combobox" aria-multiline="true">
Correct, according to the spec and what I would expect as well..
> 
> 
> How about you call your helper method IsTextOrSearchField to make it 
> clear that we are excluding combo box on purpose?
Do you think of a combobox as editable? I definitely don't, there is no caret
there.

> 
> 
> 
> > >
> > > Could you make use of the already existing method
> > > bool AXNodeObject::IsTextControl() const
> > > Or modify that one if it doesn't suit your needs.
> > I don't see how to modify it without making things more complicated. 
> > This seems
> > simpler to me.
> >
> Okay, but could we call the variable IsTextOrSearchField or something to 
> make it clear that we are excluding combo boxes on purpose?
As mentioned above, I like the current name because editing implies a caret, and
the combo box does not have one nor is editable.

> 
> > >
> > > + if (is_edit_box)
> > > + return false;
> > > Is this actually correct?
> > > How about:
> > > <div contentEditable role="textbox">
> > > Shouldn't this be multiline by default?
> > I would have guessed that too, but according to CORE-AAM, 
> > aria-multiline is
> > false by default.
> 
> Can we put a comment explaining that, because I or someone else might 
> forget and change it back in the future?
Ok, I will add a comment, but we should probably be checking specs before we
make changes -- plus tests would fail.

> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[aleventhal, 2017-06-26 19:31:25]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 19:31:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blink-reviews, 2017-06-26 19:31:47]

Re combo box.
Yes, I would think that a combo box is editable because when I go to 
google.com and find the search field, NVDA and Jaws say "Edit combo". 
The quick navigation key "c" also works and jumps to the combo box on 
google.com.
Isn't a combo box by definition a combination box? Combination between 
an edit field and a menu? At least on Win?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-26 19:31:49]

Re combo box.
Yes, I would think that a combo box is editable because when I go to 
google.com and find the search field, NVDA and Jaws say "Edit combo". 
The quick navigation key "c" also works and jumps to the combo box on 
google.com.
Isn't a combo box by definition a combination box? Combination between 
an edit field and a menu? At least on Win?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aleventhal, 2017-06-26 20:00:47]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 20:01:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nektarios, 2017-06-26 20:12:07]

lgtm

[nektarios, 2017-06-26 20:13:48]

Aaron will look into how standard compliant combo boxes should be structured.
For now some TODOs will suffice.

[aleventhal, 2017-06-26 20:31:15]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 20:31:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 21:47:49]

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

[commit-bot: I haz the power, 2017-06-26 21:47:51]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[aleventhal, 2017-06-27 13:44:42]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 13:44:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-27 15:45:55]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 15:46:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-27 16:40:55]

aleventhal@chromium.org changed reviewers:
+ dtseng@chromium.org

[aleventhal, 2017-06-27 17:06:04]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 17:06:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-06-27 17:23:38]

Hey Aaron, some things you may want to consider.  Also note, that this is going
to conflict with CL 2943243002.  We should coordinate who goes first.

https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:133: }
Note in CL 2962453002, I started moving this kind of stuff into
ax_role_properties.h. Also, in the move from BrowserAccessibility ->
AXPlatformNode, we're not going to have these helpers on BrowserAccessibility. 
I can fix this up later, but if 2962453002 lands first, I'd recommend not adding
more helpers.

https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:997: bool
BrowserAccessibility::IsEditBox() const {
Same.

[blink-reviews, 2017-06-27 17:26:44]

On 6/27/2017 1:23 PM, dougt@chromium.org wrote:
> Hey Aaron, some things you may want to consider. Also note, that this 
> is going
> to conflict with CL 2943243002. We should coordinate who goes first.
>
>
>
https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
> File content/browser/accessibility/browser_accessibility.cc (right):
>
>
https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
> content/browser/accessibility/browser_accessibility.cc:133: }
> Note in CL 2962453002, I started moving this kind of stuff into
> ax_role_properties.h. Also, in the move from BrowserAccessibility ->
> AXPlatformNode, we're not going to have these helpers on
> BrowserAccessibility. I can fix this up later, but if 2962453002 lands
> first, I'd recommend not adding more helpers.
>
>
https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
> content/browser/accessibility/browser_accessibility.cc:997: bool
> BrowserAccessibility::IsEditBox() const {
> Same.

Where should the helpers go now? AXPlatformNodeBase? Because I would 
rather not add "if" statements that check the same thing if a helper 
could be used both on Win and Mac, and possibly Android.
Having to check for the presence of text fields is common on all platforms.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-27 17:26:45]

On 6/27/2017 1:23 PM, dougt@chromium.org wrote:
> Hey Aaron, some things you may want to consider. Also note, that this 
> is going
> to conflict with CL 2943243002. We should coordinate who goes first.
>
>
>
https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
> File content/browser/accessibility/browser_accessibility.cc (right):
>
>
https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
> content/browser/accessibility/browser_accessibility.cc:133: }
> Note in CL 2962453002, I started moving this kind of stuff into
> ax_role_properties.h. Also, in the move from BrowserAccessibility ->
> AXPlatformNode, we're not going to have these helpers on
> BrowserAccessibility. I can fix this up later, but if 2962453002 lands
> first, I'd recommend not adding more helpers.
>
>
https://codereview.chromium.org/2931893002/diff/450001/content/browser/access...
> content/browser/accessibility/browser_accessibility.cc:997: bool
> BrowserAccessibility::IsEditBox() const {
> Same.

Where should the helpers go now? AXPlatformNodeBase? Because I would 
rather not add "if" statements that check the same thing if a helper 
could be used both on Win and Mac, and possibly Android.
Having to check for the presence of text fields is common on all platforms.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-06-27 18:41:14]

https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
(right):

https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:182: if
(this.getLineBreaks_().length) {
This isn't equivalent. Whether something is not multiline isn't the same as
whether the text field has no line breaks

https://codereview.chromium.org/2931893002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2931893002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:816: // Default
for other edit boxes i false, including for ARIA, says CORE-AAM.
nit: is

https://codereview.chromium.org/2931893002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:839: // can
contain a field but should not be considered edit controls themselves.
Why not? <input type="text" role="combobox">
has no children in the accessibility tree.

[blink-reviews, 2017-06-27 18:52:35]

On 6/27/2017 2:41 PM, dtseng@chromium.org wrote:
>
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> (right):
>
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:182:
> if (this.getLineBreaks_().length) {
> This isn't equivalent. Whether something is not multiline isn't the same
> as whether the text field has no line breaks
>
I think we are missing a not and a return?
if (!this.getLineBreaks_().length) {
return ;
}

That's what I meant it to be in my review.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-27 18:52:35]

On 6/27/2017 2:41 PM, dtseng@chromium.org wrote:
>
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> (right):
>
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:182:
> if (this.getLineBreaks_().length) {
> This isn't equivalent. Whether something is not multiline isn't the same
> as whether the text field has no line breaks
>
I think we are missing a not and a return?
if (!this.getLineBreaks_().length) {
return ;
}

That's what I meant it to be in my review.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-06-27 19:17:17]

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

[commit-bot: I haz the power, 2017-06-27 19:17:18]

Dry run: This issue passed the CQ dry run.

[aleventhal, 2017-06-27 19:25:16]

On 2017/06/27 18:41:14, David Tseng wrote:
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> (right):
> 
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:182:
if
> (this.getLineBreaks_().length) {
> This isn't equivalent. Whether something is not multiline isn't the same as
> whether the text field has no line breaks
The aria-multiline attribute is just a hint that the author may or may not
remember to provide, and tells the user what Enter/down arrow will do. It's more
accurate to base the presentation of lines on whether or not there actually are
multiple lines.

> 
>
https://codereview.chromium.org/2931893002/diff/450001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):
> 
>
https://codereview.chromium.org/2931893002/diff/450001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:816: //
Default
> for other edit boxes i false, including for ARIA, says CORE-AAM.
> nit: is
Fixed

> 
>
https://codereview.chromium.org/2931893002/diff/450001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:839: // can
> contain a field but should not be considered edit controls themselves.
> Why not? <input type="text" role="combobox">
> has no children in the accessibility tree.

According to the ARIA specification, a combobox is not a text field but can
contain one. Our implementation has diverged from this, and I have a TODO to
look further into this.

[aleventhal, 2017-06-27 19:26:25]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 19:26:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2017-06-27 19:34:39]

On Tue, Jun 27, 2017 at 12:25 PM, <aleventhal@chromium.org> wrote:

> On 2017/06/27 18:41:14, David Tseng wrote:
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> > File chrome/browser/resources/chromeos/chromevox/cvox2/
> background/editing.js
> > (right):
> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js#
> newcode182
> > chrome/browser/resources/chromeos/chromevox/cvox2/
> background/editing.js:182:
> if
> > (this.getLineBreaks_().length) {
> > This isn't equivalent. Whether something is not multiline isn't the same
> as
> > whether the text field has no line breaks
> The aria-multiline attribute is just a hint that the author may or may not
> remember to provide, and tells the user what Enter/down arrow will do.
> It's more
> accurate to base the presentation of lines on whether or not there
> actually are
> multiple lines.
>
> Well, this is inaccurate because the multiline attribute isn't about aria
specifically but that a text field is capable of having multiple lines.Any
other definitionis a regression from the perspective of ChromeVox.




> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
> > File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
> (right):
> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> third_party/WebKit/Source/modules/accessibility/
> AXNodeObject.cpp#newcode816
> > third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:816: //
> Default
> > for other edit boxes i false, including for ARIA, says CORE-AAM.
> > nit: is
> Fixed
>
> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> third_party/WebKit/Source/modules/accessibility/
> AXNodeObject.cpp#newcode839
> > third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:839:
> // can
> > contain a field but should not be considered edit controls themselves.
> > Why not? <input type="text" role="combobox">
> > has no children in the accessibility tree.
>
> According to the ARIA specification, a combobox is not a text field but can
> contain one. Our implementation has diverged from this, and I have a TODO
> to
> look further into this.
>
> https://codereview.chromium.org/2931893002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-06-27 19:34:40]

On Tue, Jun 27, 2017 at 12:25 PM, <aleventhal@chromium.org> wrote:

> On 2017/06/27 18:41:14, David Tseng wrote:
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> > File chrome/browser/resources/chromeos/chromevox/cvox2/
> background/editing.js
> > (right):
> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js#
> newcode182
> > chrome/browser/resources/chromeos/chromevox/cvox2/
> background/editing.js:182:
> if
> > (this.getLineBreaks_().length) {
> > This isn't equivalent. Whether something is not multiline isn't the same
> as
> > whether the text field has no line breaks
> The aria-multiline attribute is just a hint that the author may or may not
> remember to provide, and tells the user what Enter/down arrow will do.
> It's more
> accurate to base the presentation of lines on whether or not there
> actually are
> multiple lines.
>
> Well, this is inaccurate because the multiline attribute isn't about aria
specifically but that a text field is capable of having multiple lines.Any
other definitionis a regression from the perspective of ChromeVox.




> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
> > File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
> (right):
> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> third_party/WebKit/Source/modules/accessibility/
> AXNodeObject.cpp#newcode816
> > third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:816: //
> Default
> > for other edit boxes i false, including for ARIA, says CORE-AAM.
> > nit: is
> Fixed
>
> >
> >
> https://codereview.chromium.org/2931893002/diff/450001/
> third_party/WebKit/Source/modules/accessibility/
> AXNodeObject.cpp#newcode839
> > third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:839:
> // can
> > contain a field but should not be considered edit controls themselves.
> > Why not? <input type="text" role="combobox">
> > has no children in the accessibility tree.
>
> According to the ARIA specification, a combobox is not a text field but can
> contain one. Our implementation has diverged from this, and I have a TODO
> to
> look further into this.
>
> https://codereview.chromium.org/2931893002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2017-06-27 19:34:49]

On 6/27/2017 3:25 PM, aleventhal@chromium.org wrote:
> On 2017/06/27 18:41:14, David Tseng wrote:
> >
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> > File 
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> > (right):
> >
> >
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> > 
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:182:
> if
> > (this.getLineBreaks_().length) {
> > This isn't equivalent. Whether something is not multiline isn't the 
> same as
> > whether the text field has no line breaks
> The aria-multiline attribute is just a hint that the author may or may not
> remember to provide, and tells the user what Enter/down arrow will do. 
> It's more
> accurate to base the presentation of lines on whether or not there 
> actually are
> multiple lines.

If I have an empty content editable with aria-multiline set and I first 
tab to it, wouldn't this code fail to say "multiline", until I press 
enter to insert a new line? But how would I know that I can press enter?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-27 19:34:50]

On 6/27/2017 3:25 PM, aleventhal@chromium.org wrote:
> On 2017/06/27 18:41:14, David Tseng wrote:
> >
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> > File 
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> > (right):
> >
> >
>
https://codereview.chromium.org/2931893002/diff/450001/chrome/browser/resourc...
> > 
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:182:
> if
> > (this.getLineBreaks_().length) {
> > This isn't equivalent. Whether something is not multiline isn't the 
> same as
> > whether the text field has no line breaks
> The aria-multiline attribute is just a hint that the author may or may not
> remember to provide, and tells the user what Enter/down arrow will do. 
> It's more
> accurate to base the presentation of lines on whether or not there 
> actually are
> multiple lines.

If I have an empty content editable with aria-multiline set and I first 
tab to it, wouldn't this code fail to say "multiline", until I press 
enter to insert a new line? But how would I know that I can press enter?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-06-27 22:27:59]

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

[commit-bot: I haz the power, 2017-06-27 22:28:01]

Dry run: This issue passed the CQ dry run.

[aleventhal, 2017-06-28 14:20:57]

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:4315:
BrowserAccessibility* parent = owner()->PlatformGetParent();
On 2017/06/23 18:27:18, dougt wrote:
> I am confused by this. 
> 
> GetParent() is just returning the node above |this| one.  Shouldn't we walk up
> the tree to the root?

It does what I want, but maybe it needs a different name.

This is looking to see if we're in an editable area AND we're in the top
editable element of that area. If the thing above us is not editable, then we
aren't in a root editable.

For example:

Section -- not editable
  Paragraph -- editable
    Link -- editable

Only the paragraph there is a root editable.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:4385: // direct
text-only children should not be present at tree  s and so no
On 2017/06/23 18:27:18, dougt wrote:
> I don't think you want this change.

Done.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:5103: }
On 2017/06/23 18:27:17, dougt wrote:
> Since this is a one liner, you don't need the curly braces.

Done.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
File content/browser/accessibility/browser_accessibility_com_win.h (right):

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.h:817: // Is the
root of an editable area
On 2017/06/23 18:27:18, dougt wrote:
> Nit: Comment isn't needed.

Done.

https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.h:820: // Is an
editbox of some type
On 2017/06/23 18:27:18, dougt wrote:
> Same.

Done.

[aleventhal, 2017-06-28 14:35:55]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 14:36:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blink-reviews, 2017-06-28 14:50:34]

>
https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
> content/browser/accessibility/browser_accessibility_com_win.h:820: // Is
> an editbox of some type
> On 2017/06/23 18:27:18, dougt wrote:
> > Same.

Isn't it better to say EditField instead of EditBox?
It's the more formal way and that's what I will be searching for when 
doing a find through the file.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-28 14:50:34]

>
https://codereview.chromium.org/2931893002/diff/280001/content/browser/access...
> content/browser/accessibility/browser_accessibility_com_win.h:820: // Is
> an editbox of some type
> On 2017/06/23 18:27:18, dougt wrote:
> > Same.

Isn't it better to say EditField instead of EditBox?
It's the more formal way and that's what I will be searching for when 
doing a find through the file.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-06-28 15:16:27]

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

[commit-bot: I haz the power, 2017-06-28 15:16:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2017-06-28 19:43:21]

https://codereview.chromium.org/2931893002/diff/490001/content/browser/access...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/2931893002/diff/490001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:134: }
I would add this to
https://cs.chromium.org/chromium/src/ui/accessibility/ax_role_properties.h. 
Same with BrowserAccessibility::IsEditBox().

https://codereview.chromium.org/2931893002/diff/490001/content/browser/access...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2931893002/diff/490001/content/browser/access...
content/browser/accessibility/browser_accessibility_com_win.cc:5334: ia_state |=
STATE_SYSTEM_READONLY;
hrm? I thought I got rid of ia_state on master.

[aleventhal, 2017-06-28 19:46:23]

> Isn't it better to say EditField instead of EditBox?
> It's the more formal way and that's what I will be searching for when 
> doing a find through the file.
> 

Ok, done.

[nektarios, 2017-06-28 19:54:52]

lgtm

[aleventhal, 2017-06-28 21:03:31]

Try to use multiline field

[aleventhal, 2017-06-28 21:03:32]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 21:04:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 22:06:26]

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

[commit-bot: I haz the power, 2017-06-28 22:06:27]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[aleventhal, 2017-06-30 14:21:09]

Tests need to specify aria-multiline in order to avoid ARIA default of
multiline=false

[aleventhal, 2017-06-30 14:21:10]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 14:21:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-30 15:50:42]

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

[commit-bot: I haz the power, 2017-06-30 15:50:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleventhal, 2017-06-30 18:41:08]

Fix test

[aleventhal, 2017-06-30 18:41:09]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 18:41:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-06-30 20:32:47]

@dtseng PTAL

[commit-bot: I haz the power, 2017-06-30 21:14:27]

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

[commit-bot: I haz the power, 2017-06-30 21:14:29]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-06-30 22:03:30]

https://codereview.chromium.org/2931893002/diff/550001/content/browser/access...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/2931893002/diff/550001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:131: bool
BrowserAccessibility::IsDocument() const {
Please do not add more simple predicates here.  Instead add them to
ax_role_properties.*

https://codereview.chromium.org/2931893002/diff/550001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:988: bool
BrowserAccessibility::IsEditField() const {
Same.

[David Tseng, 2017-07-01 01:17:41]

https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
(right):

https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:157: if
(breaks.length < 1)
Thanks for the cleanup, but this entire class is pending removal.

If you want to do this, then also make @return {!Array...} for getLineBreaks_
and just check length is 0 here. Length is never < 0.

https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs
(right):

https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:202:
<div role="textbox" contenteditable aria-multiline="true">
This is a pretty bad example to do this. The contenteditable clearly has
multiple lines and no event listeners to intercept key events.

The practical result of this is that ChromeVox can no longer rely upon the
multiline attribute for anything in contenteditables.

I think what would be a better justification of this change is to try collecting
some stats of real world usage. How many pages have contenteditables with role
textbox and how many of them are single lined?

Also, even if a page author intercepts all key events, if I type enough text
into the content editable, it will wrap (making it a multi line value). A author
would have to go pretty far to make the content editable "single" lined.
I could also paste multiple lines into the contet editable.

So, basically an author would have to:
- intercept at least enter (key event)
- make the content editable not wrap
- intercept paste
- hope no one inserts text with a content script

[aleventhal, 2017-07-05 18:49:51]

On 2017/07/01 01:17:41, David Tseng wrote:
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> (right):
> 
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:157:
if
> (breaks.length < 1)
> Thanks for the cleanup, but this entire class is pending removal.
> 
> If you want to do this, then also make @return {!Array...} for getLineBreaks_
> and just check length is 0 here. Length is never < 0.
> 
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> File
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs
> (right):
> 
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:202:
> <div role="textbox" contenteditable aria-multiline="true">
> This is a pretty bad example to do this. The contenteditable clearly has
> multiple lines and no event listeners to intercept key events.
> 
> The practical result of this is that ChromeVox can no longer rely upon the
> multiline attribute for anything in contenteditables.
> 
> I think what would be a better justification of this change is to try
collecting
> some stats of real world usage. How many pages have contenteditables with role
> textbox and how many of them are single lined?
> 
> Also, even if a page author intercepts all key events, if I type enough text
> into the content editable, it will wrap (making it a multi line value). A
author
> would have to go pretty far to make the content editable "single" lined.
> I could also paste multiple lines into the contet editable.
> 
> So, basically an author would have to:
> - intercept at least enter (key event)
> - make the content editable not wrap
> - intercept paste
> - hope no one inserts text with a content script

I don't think we should try to rethink a spec that is already relied on by
authors and implemented correctly in Firefox. I could understand wanting to have
a different default when no role is used, if other browser vendors agreed.

[aleventhal, 2017-07-06 13:24:46]

Rebase

[aleventhal, 2017-07-06 13:24:47]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 13:25:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-07-06 13:25:22]

On 2017/07/05 18:49:51, aleventhal wrote:
> On 2017/07/01 01:17:41, David Tseng wrote:
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> > File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> > chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:157:
> if
> > (breaks.length < 1)
> > Thanks for the cleanup, but this entire class is pending removal.
> > 
> > If you want to do this, then also make @return {!Array...} for
getLineBreaks_
> > and just check length is 0 here. Length is never < 0.
> > 
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> > File
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs
> > (right):
> > 
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:202:
> > <div role="textbox" contenteditable aria-multiline="true">
> > This is a pretty bad example to do this. The contenteditable clearly has
> > multiple lines and no event listeners to intercept key events.
> > 
> > The practical result of this is that ChromeVox can no longer rely upon the
> > multiline attribute for anything in contenteditables.
> > 
> > I think what would be a better justification of this change is to try
> collecting
> > some stats of real world usage. How many pages have contenteditables with
role
> > textbox and how many of them are single lined?
> > 
> > Also, even if a page author intercepts all key events, if I type enough text
> > into the content editable, it will wrap (making it a multi line value). A
> author
> > would have to go pretty far to make the content editable "single" lined.
> > I could also paste multiple lines into the contet editable.
> > 
> > So, basically an author would have to:
> > - intercept at least enter (key event)
> > - make the content editable not wrap
> > - intercept paste
> > - hope no one inserts text with a content script
> 
> I don't think we should try to rethink a spec that is already relied on by
> authors and implemented correctly in Firefox. I could understand wanting to
have
> a different default when no role is used, if other browser vendors agreed.

@dtseng, PTAL

[aleventhal, 2017-07-06 13:25:30]

On 2017/07/05 18:49:51, aleventhal wrote:
> On 2017/07/01 01:17:41, David Tseng wrote:
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> > File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> > chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:157:
> if
> > (breaks.length < 1)
> > Thanks for the cleanup, but this entire class is pending removal.
> > 
> > If you want to do this, then also make @return {!Array...} for
getLineBreaks_
> > and just check length is 0 here. Length is never < 0.
> > 
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> > File
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs
> > (right):
> > 
> >
>
https://codereview.chromium.org/2931893002/diff/550001/chrome/browser/resourc...
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:202:
> > <div role="textbox" contenteditable aria-multiline="true">
> > This is a pretty bad example to do this. The contenteditable clearly has
> > multiple lines and no event listeners to intercept key events.
> > 
> > The practical result of this is that ChromeVox can no longer rely upon the
> > multiline attribute for anything in contenteditables.
> > 
> > I think what would be a better justification of this change is to try
> collecting
> > some stats of real world usage. How many pages have contenteditables with
role
> > textbox and how many of them are single lined?
> > 
> > Also, even if a page author intercepts all key events, if I type enough text
> > into the content editable, it will wrap (making it a multi line value). A
> author
> > would have to go pretty far to make the content editable "single" lined.
> > I could also paste multiple lines into the contet editable.
> > 
> > So, basically an author would have to:
> > - intercept at least enter (key event)
> > - make the content editable not wrap
> > - intercept paste
> > - hope no one inserts text with a content script
> 
> I don't think we should try to rethink a spec that is already relied on by
> authors and implemented correctly in Firefox. I could understand wanting to
have
> a different default when no role is used, if other browser vendors agreed.

@dtseng, PTAL

[commit-bot: I haz the power, 2017-07-06 15:29:43]

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

[commit-bot: I haz the power, 2017-07-06 15:29:45]

Dry run: This issue passed the CQ dry run.

[aleventhal, 2017-07-07 11:59:13]

Rebase and address comments

[aleventhal, 2017-07-07 11:59:15]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 11:59:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-07-07 12:18:59]

Dtseng, it's rebased and ready. PTAL.

[commit-bot: I haz the power, 2017-07-07 14:15:56]

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

[commit-bot: I haz the power, 2017-07-07 14:15:58]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-07-07 17:58:06]

lgtm for small ChromeVox change. nektar should review platform specific code; I
can if he's not available.

https://codereview.chromium.org/2931893002/diff/570001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
(right):

https://codereview.chromium.org/2931893002/diff/570001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:157: if
(breaks.length < 1)
These changes should be unneeded now.

[aleventhal, 2017-07-07 19:09:28]

On 2017/07/07 17:58:06, David Tseng wrote:
> lgtm for small ChromeVox change. nektar should review platform specific code;
I
> can if he's not available.
Thanks, nektar already reviewed.

[aleventhal, 2017-07-07 19:10:49]

Remove unnecessary changes

[aleventhal, 2017-07-07 19:10:50]

The CQ bit was checked by aleventhal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 19:11:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-07-07 19:11:42]

The CQ bit was unchecked by aleventhal@chromium.org

[aleventhal, 2017-07-07 19:11:55]

The CQ bit was checked by aleventhal@chromium.org

[aleventhal, 2017-07-07 19:11:56]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2931893002/#ps610001
(title: "Remove unnecessary changes")

[commit-bot: I haz the power, 2017-07-07 19:12:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 21:17:44]

CQ is committing da patch.

Bot data: {"patchset_id": 610001, "attempt_start_ts": 1499454715884540,
"parent_rev": "a39d6098709c2efec55e8832ae3a17ec428350d4", "commit_rev":
"10b1109333ea048db0011af36e751f8d3c996ed2"}

[commit-bot: I haz the power, 2017-07-07 21:17:59]

CQ is committing da patch.

Bot data: {"patchset_id": 610001, "attempt_start_ts": 1499454715884540,
"parent_rev": "47d872925a8504668a2b2c65f1d0572264d08398", "commit_rev":
"b6267a380ae52cc410cce9752bdae27c34105d36"}

[commit-bot: I haz the power, 2017-07-07 21:18:19]

CQ is committing da patch.

Bot data: {"patchset_id": 610001, "attempt_start_ts": 1499454715884540,
"parent_rev": "61d337ac372af7ad2eb77d19dc768b5185b6b14d", "commit_rev":
"5a1e7c0f36fc682b8572f3d781943aef6edce818"}

[commit-bot: I haz the power, 2017-07-07 21:18:35]

Description was changed from

==========
More precise use of multiline state. It only needs to be exposed at the root of
the editable area. ARIA should override native markup. Add lots of tests.

BUG=730138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
More precise use of multiline state. It only needs to be exposed at the root of
the editable area. ARIA should override native markup. Add lots of tests.

BUG=730138
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2931893002
Cr-Commit-Position: refs/heads/master@{#485051}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a1e7c0f36fc682b8572f3d78194...
==========

[commit-bot: I haz the power, 2017-07-07 21:18:40]

Committed patchset #32 (id:610001) as
https://chromium.googlesource.com/chromium/src/+/5a1e7c0f36fc682b8572f3d78194...