Added layout test for aria-orientation and cleaned up code. Also exposed hr element with an orienta…

Added layout test for aria-orientation and cleaned up code. Also exposed hr element with an orientation of horizontal.

BUG=530748
R=dmazzoni@chromium.org

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

[dmazzoni, 2015-09-15 22:43:01]

lgtm with one suggestion for a more concise test

https://codereview.chromium.org/1346733002/diff/1/LayoutTests/accessibility/a...
File LayoutTests/accessibility/aria-orientation.html (right):

https://codereview.chromium.org/1346733002/diff/1/LayoutTests/accessibility/a...
LayoutTests/accessibility/aria-orientation.html:60: if (i == 1 || i == 3)
How about if the intended orientation is just an attribute on the element? For
example:

<div id="slider1" role="slider"
   aria-valuemin="0" aria-valuemax="100" aria-valuenow="50"
   expected-orientation="AXOrientation: AXHorizontalOrientation">
   50
 </div>

shouldBe(axSlider.orientation, slider.getAttribute('expected-orientation'))

Then instead of iterating over each type of element, you could just iterate over
all elements on the page with an expected-orientation attribute.

[blink-reviews, 2015-09-16 17:50:36]

Done. But code was also updated.
Could you look at the original code in AXLayoutObject.cpp and in 
particular the comment that says that "TreeGridRole is overwritten in 
AXTable". I couldn't find any code in AXTable.cpp that overwrites the 
TreeGridRole, hence I didn't do anything special in my code. Still I 
want to understand the purpose of that comment in the original code. I 
might be missing something.

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

[dmazzoni, 2015-09-16 21:43:48]

Your code looks fine.

Hmmm, that does seem to be problematic, that AXTable::roleValue always
returns TableRole, overriding the ARIA role. Let's look into that as a
separate bug.


On Wed, Sep 16, 2015 at 10:50 AM Nektarios Paisios <nektar@google.com>
wrote:

> Done. But code was also updated.
> Could you look at the original code in AXLayoutObject.cpp and in
> particular the comment that says that "TreeGridRole is overwritten in
> AXTable". I couldn't find any code in AXTable.cpp that overwrites the
> TreeGridRole, hence I didn't do anything special in my code. Still I
> want to understand the purpose of that comment in the original code. I
> might be missing something.
>
>

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

[blink-reviews, 2015-09-16 22:36:36]

Okay, I filed a bug (532670).
Also, I added one final test case.
<div role="button" aria-orientation="vertical">
Expected orientation should be undefined since it doesn't make sense to 
have an ARIA button with an orientation.

I'll submit now.

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

[nektarios, 2015-09-16 22:36:56]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-09-16 22:36:56]

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/1346733002/#ps60001
(title: "Added a final test case.")

[commit-bot: I haz the power, 2015-09-16 22:37:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346733002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346733002/60001

[commit-bot: I haz the power, 2015-09-16 23:20:31]

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

[commit-bot: I haz the power, 2015-09-16 23:20:32]

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

[blink-reviews, 2015-09-17 17:08:33]

I'll add the table role in temporarily until I fix the bug.

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

[nektarios, 2015-09-17 17:11:42]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-09-17 17:11:42]

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/1346733002/#ps80001
(title: "Added TableRole back with a TODO to fix the relevant bug.")

[commit-bot: I haz the power, 2015-09-17 17:11:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346733002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346733002/80001

[commit-bot: I haz the power, 2015-09-17 18:26:44]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202461