Bug 118106 – Web Inspector: AX: Add additional contextual labels for Error/Warnings/Logs that will be spoken for screen readers

• Component: Web Inspector
• Status: RESOLVED FIXED

[James Craig]

Web Inspector: AX: Add additional contextual labels for Error/Warnings/Logs that will be spoken for screen readers

Icons next to each line in the console log indicate if it's an error, warning, or log. These are just background images, so look for "data-labelprefix" in the source to see how this is done for the "Input:" and "Output:" strings.

[Radar WebKit Bug Importer]

<rdar://problem/14284013>

[James Craig]

Dependent on bug 115976

[James Craig]

Created attachment 208968 [details]
patch

[Joseph Pecoraro]

Comment on attachment 208968 [details]
patch

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

Nice!

> Source/WebInspectorUI/UserInterface/ConsoleCommandResult.js:49
> +        if (!element.getAttribute("data-labelprefix")) 

Nit: This check could be hasAttribute instead of getAttribute.

[Joseph Pecoraro]

Comment on attachment 208968 [details]
patch

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

> Source/WebInspectorUI/UserInterface/ConsoleMessageImpl.js:486
> +                element.setAttribute("data-labelprefix", WebInspector.UIString("Tip: "));

We could even use elem.dataset for "data-x" attributes:

    element.dataset.labelprefix = WebInspector.UIString("Tip: ");

However, I'm not sure if dataset is any clearer or more performant. So lets stick with your setAttribute calls. It makes it easy to search for "data-labelprefix" and find all the places we deal with it in code.

[James Craig]

(In reply to comment #4)
> (From update of attachment 208968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208968&action=review
> 
> Nice!
> 
> > Source/WebInspectorUI/UserInterface/ConsoleCommandResult.js:49
> > +        if (!element.getAttribute("data-labelprefix")) 
> 
> Nit: This check could be hasAttribute instead of getAttribute.

hasAttribute won't cover the empty string case, I believe. Without my shorthand version, I'd need to do this to cover both cases.

if (!element.hasAttribute("data-labelprefix") || element.getAttribute("data-labelprefix") !== "") 

But I like the shorthand version better.

[James Craig]

(In reply to comment #5)
> (From update of attachment 208968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208968&action=review
> 
> > Source/WebInspectorUI/UserInterface/ConsoleMessageImpl.js:486
> > +                element.setAttribute("data-labelprefix", WebInspector.UIString("Tip: "));
> 
> We could even use elem.dataset for "data-x" attributes:
> 
>     element.dataset.labelprefix = WebInspector.UIString("Tip: ");
> 
> However, I'm not sure if dataset is any clearer or more performant. So lets stick with your setAttribute calls. It makes it easy to search for "data-labelprefix" and find all the places we deal with it in code.

Good idea. I could go either way on this, but it looks like you're leaning slightly toward searchability, so I'll leave it the way it is for now.

[WebKit Commit Bot]

Comment on attachment 208968 [details]
patch

Clearing flags on attachment: 208968

Committed r154310: <http://trac.webkit.org/changeset/154310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.