Expose html attributes in automation tree.

Expose html attributes in automation tree.

BUG=624124
Committed: https://crrev.com/cb1959dadc49ec789b0588c64cd85e142cd0fb48
Cr-Commit-Position: refs/heads/master@{#402990}

[David Tseng, 2016-06-10 21:18:00]

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

[dmazzoni, 2016-06-13 16:22:35]

Hmmm, I'm lukewarm on this one due to efficiency and due to it being a leaky
abstraction.

Either way, please add tagName for completeness

How about an async function to fetch the tagName and attributes instead? Or a
function to fetch outerHTML? Even if it was implemented the same way behind the
scenes now, that would (1) allow for the possibility of lazily fetching them in
the future, and (2) discourage abusing this API as a way to create simple output
rules and allow it only for special things like MathML, or maybe for debugging /
introspection

[David Tseng, 2016-06-13 16:59:40]

On 2016/06/13 16:22:35, dmazzoni wrote:
> Hmmm, I'm lukewarm on this one due to efficiency and due to it being a leaky
> abstraction.

I'm open to coming up with something less leaky...but these attributes also get
set directly by Windows. I'm not sure what the efficiency concern is. The
attributes are only fetched if ChromeVox asks for them right? 

> 
> Either way, please add tagName for completeness

That doesn't exist; do you mean html_tag? What about display (and various other
attributes that are really html attributes)?

I think we need to agree on what abstraction you're referring to. ax_enums.idl
is our abstraction as far as I know. If you have another abstraction, we should
declare it and agree on it.
> 
> How about an async function to fetch the tagName and attributes instead?

Design-wise, the automation API sets attributes directly. It would be odd to
have a special attribute getter for these attributes alone.

 Or a
> function to fetch outerHTML? Even if it was implemented the same way behind
the
> scenes now, that would (1) allow for the possibility of lazily fetching them
in
> the future, and (2) discourage abusing this API as a way to create simple
output
> rules and allow it only for special things like MathML, or maybe for debugging
/
> introspection

Fetching outerHTML as a string? Seems wasteful to serialize to that format and
then reserialize inside of ChromeVox.

We could add an attribute whitelist if abuse is really a concern but it seems
only like an issue if we plan on making the api public.

I can start an *Internal variant of every attribute type or something like it.
The only issue is we do expose many of these attributes on other platforms
directly. In other words, I don't want to artificially limit what data ChromeVox
has access to.

[dmazzoni, 2016-06-13 19:02:23]

On Mon, Jun 13, 2016 at 9:59 AM <dtseng@chromium.org> wrote:

> I'm open to coming up with something less leaky...but these attributes
> also get
> set directly by Windows. I'm not sure what the efficiency concern is. The
> attributes are only fetched if ChromeVox asks for them right?
>

Ideally I'd like to not even fetch them from Blink.

For Windows I'm hoping to get a list of attributes that JAWS & NVDA
actually care about and not fetch anything but those.


> That doesn't exist; do you mean html_tag? What about display (and various
> other
> attributes that are really html attributes)?
>

Yes, html_tag.

We should get rid of display, probably - I need to sync with FS & NV Access
and see which ones they use and why.


> I think we need to agree on what abstraction you're referring to.
> ax_enums.idl
> is our abstraction as far as I know. If you have another abstraction, we
> should
> declare it and agree on it.
>

I feel like the automation API is an attempt to abstract the accessibility
tree slightly more, i.e. we try to keep ourselves honest by not exposing
legacy cruft or hacks for one platform, especially since we might want to
make this API public someday.


> > How about an async function to fetch the tagName and attributes instead?
>
> Design-wise, the automation API sets attributes directly. It would be odd
> to
> have a special attribute getter for these attributes alone.
>

No, it used to. The current implementation iterates over the attributes and
adds a getter to the AutomationNode prototype - but when you call it, it
calls into C++ code to get the value of an attribute dynamically each time.

This would basically just mean creating a new category of attributes that
are fetched via an asynchronous method rather than via a getter.

Fetching outerHTML as a string? Seems wasteful to serialize to that format
> and
> then reserialize inside of ChromeVox.
>

For MathML one thought is that we could actually stick that HTML somewhere
to run MathJAX on it locally. In that case I'd argue it'd be more efficient
to do that rather than to build DOM elements from tag names and lists of
attributes.

-- 
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.

[chromium-reviews, 2016-06-13 20:41:34]

On Mon, Jun 13, 2016 at 12:02 PM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Mon, Jun 13, 2016 at 9:59 AM <dtseng@chromium.org> wrote:
>
>> I'm open to coming up with something less leaky...but these attributes
>> also get
>> set directly by Windows. I'm not sure what the efficiency concern is. The
>> attributes are only fetched if ChromeVox asks for them right?
>>
>
> Ideally I'd like to not even fetch them from Blink.
>
> For Windows I'm hoping to get a list of attributes that JAWS & NVDA
> actually care about and not fetch anything but those.
>
Ok; we're in the same position then with regard to data-mathml and probably
a bunch of others. I can wait for that, but in the meantime, this cl
probably won't hurt.


>
>
>> That doesn't exist; do you mean html_tag? What about display (and various
>> other
>> attributes that are really html attributes)?
>>
>
> Yes, html_tag.
>

That's already plumbed through then.


>
> We should get rid of display, probably - I need to sync with FS & NV
> Access and see which ones they use and why.
>
>
>> I think we need to agree on what abstraction you're referring to.
>> ax_enums.idl
>> is our abstraction as far as I know. If you have another abstraction, we
>> should
>> declare it and agree on it.
>>
>
> I feel like the automation API is an attempt to abstract the accessibility
> tree slightly more, i.e. we try to keep ourselves honest by not exposing
> legacy cruft or hacks for one platform, especially since we might want to
> make this API public someday.
>



>
>
>> > How about an async function to fetch the tagName and attributes instead?
>>
>> Design-wise, the automation API sets attributes directly. It would be odd
>> to
>> have a special attribute getter for these attributes alone.
>>
>
> No, it used to. The current implementation iterates over the attributes
> and adds a getter to the AutomationNode prototype - but when you call it,
> it calls into C++ code to get the value of an attribute dynamically each
> time.
>
> I understand that. The direct-nesss is that the client api is *not* async.
The html attributes is also a getter and calls through to C++ when
requested; it's very similar to the way we handle other objects like state.



This would basically just mean creating a new category of attributes that
> are fetched via an asynchronous method rather than via a getter.
>
> So, what you want is:
AutomationNode.prototype.getAttribute(attributeName, callback)

and on the native side, we would have to linearly scan the attributes for
attributeName?  I'm not convinced that's any better if our goal is to have
a well defined set of attributes.



Fetching outerHTML as a string? Seems wasteful to serialize to that format
>> and
>> then reserialize inside of ChromeVox.
>>
>
> For MathML one thought is that we could actually stick that HTML somewhere
> to run MathJAX on it locally. In that case I'd argue it'd be more efficient
> to do that rather than to build DOM elements from tag names and lists of
> attributes.
>
>
To be clear, the data-mathml attribute is a string of xml. The mathml tree
isn't actually in the DOM.

We would pass that mathml string directly to mathjax.

Since this change seems controversial, let's discuss offline.



> --
> 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.

[chromium-reviews, 2016-06-14 05:38:28]

On Mon, Jun 13, 2016 at 1:41 PM 'David Tseng' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> To be clear, the data-mathml attribute is a string of xml. The mathml tree
> isn't actually in the DOM.
>

So it's just one attribute we need? I was thinking it was the whole
generated HTML subtree for the math.

Let's chat about it tomorrow. Maybe we can just try to create somewhere to
keep track of any usage of HTML tag names and attributes in ChromeVox code
all in one place so that when we do future optimizations like I want to do
for Windows we can be sure all HTML tags and attributes we actually use are
all in one place.

-- 
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, 2016-06-14 15:42:14]

On 2016/06/14 05:38:28, chromium-reviews wrote:
> On Mon, Jun 13, 2016 at 1:41 PM 'David Tseng' via Chromium-reviews <
> mailto:chromium-reviews@chromium.org> wrote:
> 
> > To be clear, the data-mathml attribute is a string of xml. The mathml tree
> > isn't actually in the DOM.
> >
> 
> So it's just one attribute we need? I was thinking it was the whole
> generated HTML subtree for the math.

Yes; that's the misunderstanding. I wasn't proposing to re-construct the entire
mathml tree from the automation tree.

Sample:

"<span id="MathJax-Element-1-Frame" class="mjx-chtml MathJax_CHTML" tabindex="0"
data-mathml="<math xmlns=&quot;http://www.w3.org/1998/Math/MathML&quot;
display=&quot;block&quot;
mathcolor=&quot;white&quot;><mrow><mi>f</mi><mrow><mo>(</mo><mi>a</mi><mo>)</mo></mrow></mrow><mo>=</mo><mrow><mfrac><mn>1</mn><mrow><mn>2</mn><mi>&amp;#x3C0;</mi><mi>i</mi></mrow></mfrac><msub><mo>&amp;#x222E;</mo><mrow><mi>&amp;#x3B3;</mi></mrow></msub><mfrac><mrow><mi>f</mi><mo>(</mo><mi>z</mi><mo>)</mo></mrow><mrow><mi>z</mi><mo>&amp;#x2212;</mo><mi>a</mi></mrow></mfrac><mi>d</mi><mi>z</mi></mrow></math>"
role="presentation" style="font-size: 120%; position: relative;"><span 
> 

> Let's chat about it tomorrow. Maybe we can just try to create somewhere to
> keep track of any usage of HTML tag names and attributes in ChromeVox code
> all in one place so that when we do future optimizations like I want to do
> for Windows we can be sure all HTML tags and attributes we actually use are
> all in one place.
> 


> -- 
> 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.

[dmazzoni, 2016-06-14 21:44:34]

lgtm

OK, I can't think of anything better in the meantime.
Let's go with this and I'll add a white list later.

[David Tseng, 2016-06-28 19:54:41]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:56:01]

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, 2016-06-28 20:04:22]

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

[commit-bot: I haz the power, 2016-06-28 20:04:23]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[David Tseng, 2016-06-29 22:01:03]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-06-29 22:02:48]

Description was changed from

==========
Expose html attributes in automation tree.
==========

to

==========
Expose html attributes in automation tree.

BUG=624124
==========

[commit-bot: I haz the power, 2016-06-29 22:02:56]

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, 2016-06-29 23:34:38]

Description was changed from

==========
Expose html attributes in automation tree.

BUG=624124
==========

to

==========
Expose html attributes in automation tree.

BUG=624124
==========

[commit-bot: I haz the power, 2016-06-29 23:34:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-29 23:35:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 23:38:22]

Description was changed from

==========
Expose html attributes in automation tree.

BUG=624124
==========

to

==========
Expose html attributes in automation tree.

BUG=624124

Committed: https://crrev.com/cb1959dadc49ec789b0588c64cd85e142cd0fb48
Cr-Commit-Position: refs/heads/master@{#402990}
==========

[commit-bot: I haz the power, 2016-06-29 23:38:23]

Patchset 1 (id:??) landed as
https://crrev.com/cb1959dadc49ec789b0588c64cd85e142cd0fb48
Cr-Commit-Position: refs/heads/master@{#402990}