See screenshot. Maybe there is a way to prevent this text from showing up on hover, or we could make this a non-link element to prevent it.

(In reply to Brian Grinstead [:bgrins] from comment #0) > Created attachment 8395955 [details] > webconsole-click-to-select-node.png > > See screenshot. Maybe there is a way to prevent this text from showing up > on hover, or we could make this a non-link element to prevent it. I know there is an arrow pointing to the icon in this screenshot, but the element I am concerned about is actually just above the devtools toolbox - the link to chrome://browser/skin/devtools/webconsole.xul#

This is happening because we use anchors in the console output.

I'm thinking it shouldn't be too big of a deal to not use anchors, and use a span or something instead. Will have to have a closer look

I can look into this.

(In reply to alexharris6 from comment #4) > I can look into this. Awesome! I'm not entirely sure where this particular node is being generated, but it has the class .open-inspector.

It'd be nice to use a focusable element. A button element might be good. Or we could just prevent the default behaviour while hovering (e.preventDefault() might work ?)

(In reply to Tim Nguyen [:ntim] from comment #6) > It'd be nice to use a focusable element. A button element might be good. Or > we could just prevent the default behaviour while hovering > (e.preventDefault() might work ?) It is currently focusable? I can't tab into it if that's what you mean. Anyhow, preventDefault() would not work for this. We could use some element like a span or a button, or could remove the href attribute. I think a button makes sense in this case.

Here is a pastebin with the actual code creating the element, and an initial attempt at a solution that doesn't work, but might be on the right track. I can keep working on it, but maybe someone has some insights. https://pastebin.mozilla.org/5262518

(In reply to alexharris6 from comment #8) > Here is a pastebin with the actual code creating the element, and an initial > attempt at a solution that doesn't work, but might be on the right track. I > can keep working on it, but maybe someone has some insights. > > https://pastebin.mozilla.org/5262518 Looking at that, I would probably modify the _anchor function to not make href='#' when href isn't passed in. This looks like it is actually an issue on all kinds of elements, not just the 'open in inspector' button. For instance, if you run `document` in the console then hover over the output it has the same issue. What happens if you change this line let anchor = this.el("a", { class: options.className, draggable: false, href: options.href || "#", }, text); To only conditionally add the href, something like this? let anchor = this.el("a", { class: options.className, draggable: false }, text); if (options.href) { anchor.setAttribute('href', options.href); }

Without the "href" attribute, the element is no longer keyboard focusable.

(In reply to Tim Nguyen [:ntim] from comment #10) > Without the "href" attribute, the element is no longer keyboard focusable. I don't really care if we conditionally use buttons instead of <a> tags without the href. I'm unable to focus any of these elements with my keyboard, but maybe I'm missing something. I think the key will be to make the changes inside of the _anchor function though, to make sure that all different types of output get the fix.

Hey, OK to making changes in _anchor function, I wasn't sure how appropriate it was to make such a broad change to resolve this specific bug. Is it reasonable to make all of these elements that do not have a specified href attribute into buttons instead of a tags, or should it have to be specifically decided each time _anchor is called with something like `button:true`?

(In reply to alexharris6 from comment #12) > Hey, OK to making changes in _anchor function, I wasn't sure how appropriate > it was to make such a broad change to resolve this specific bug. Is it > reasonable to make all of these elements that do not have a specified href > attribute into buttons instead of a tags, or should it have to be > specifically decided each time _anchor is called with something like > `button:true`? Good question - I think there is no point in using an <a> tag in any case where there is no href, as it will cause this same issue. So, ideally we could make the change globally for anything non passing in an href. However, I'm not sure if tests are relying on this particular markup so if making the change globally causes things to blow up, then we could back off and only optionally change the markup. There is the issue of a function named '_anchor' creating a different tag name, but changing the function name could break other things (it is documented on this page: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Custom_output).

(In reply to Brian Grinstead [:bgrins] from comment #13) > Good question - I think there is no point in using an <a> tag in any case > where there is no href, as it will cause this same issue. So, ideally we > could make the change globally for anything non passing in an href. > However, I'm not sure if tests are relying on this particular markup so if > making the change globally causes things to blow up, then we could back off > and only optionally change the markup. Actually, after talking to robcee about this, I'm in favor of going with the <a> and no href instead of <button> personally. The reason is that we do not currently support keyboard navigation / focus between output nodes in the console. And this change seems like it would limit the scope of the fix and have less chance of causing accidental changes in the UI of the output. Related: here is a list of bugs opened for improving the accessibility support in the tools: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=comp%3Adeveloper%20a11y&list_id=10323273

Hmm, well maybe this is an OSX-only bug that is preventing the keyboard focus of these elements: Bug 979275. Alex, can you upload a patch in which no href is added if none was passed in and then we can test it on Windows/Linux to see if it breaks things there?

Here is a patch that only sets an href attribute on an <a> tag created with the _anchor function if an href is explicitly passed in. As seen in comment #8. This seems to be working for me in OS X. I can hover over the node with no url appearing, and clicking has the intended effect.

Comment on attachment 8431911 [details] [diff] [review] Patch for 987373 Review of attachment 8431911 [details] [diff] [review]: ----------------------------------------------------------------- This definitely fixes the problem as described. From what ntim is saying in Comment 10, this will not allow keyboard focus throughout the output. Tim, can you check and see if this indeed breaks keyboard functionality for you? On OSX it doesn't change the behavior, but it seems it not keyboard focusable to start with.

Comment on attachment 8431911 [details] [diff] [review] Patch for 987373 Review of attachment 8431911 [details] [diff] [review]: ----------------------------------------------------------------- This breaks keyboard accessibility on Windows :( Note that a <button> element seems best to me, since the tabindex attribute can give weird behaviour.

Alex, can you modify the patch to create a button instead of an <a> tag if no `href` was passed in? This may require some style changes (possibly -moz-appearance: none) in themes/shared/devtools/webconsole.inc.css

I have modified the JS to make the "anchor" a `<button>` element if href is not set, as seen here: https://pastebin.mozilla.org/5439718 The results of this are seen in the screenshot attached. It appears that many of the anchors in the console are affected by this change, changing them all into buttons. I guess this is fine if the styles are modified and they continue to work, but it seems a bit heavy handed to me. Also, I tried adding -moz-appearance: none to the .open-inspector and it didn't seem to do anything. Manually removing the border with `border: 0` does work, although width and height for open-inspector will need to be manually declared, as well as removing the existing padding/margin. I didn't want to delve to deeply into this until we get the basic approach nailed down, then I can fine tune the CSS.

Looking at that, I suspect that we will be able to do the styling we want. I noticed that the <h1> node is being printed out as a link currently, and with the syntax highlighting going on in these nodes, that means so we do deal with nested nodes. For instance, if you open this page on bugzilla and run document.getElementsByTagName("body"), there is markup like this: <a class="cm-tag" draggable="false" href="#">body<span class="cm-attribute">.bugzilla-mozilla-org.skin-Mozilla.bz_bug.bz_status_ASSIGNED.bz_product_Firefox.bz_component_Developer_Tools:_Console.bz_bug_987373.bz_gravatar.yui-skin-sam.git_style.bzJS-commentoverflow</span></a> However, I think this could be styled like so: http://jsfiddle.net/bgrins/Z94MB/ (I haven't tested within the web console, but I think it would be worth trying).

Here is a patch that modifies the _anchor function so that it creates a button if options.href is empty, as well adds some CSS to fix styling for the created buttons.

Comment on attachment 8443740 [details] [diff] [review] Patch for 987373 Review of attachment 8443740 [details] [diff] [review]: ----------------------------------------------------------------- The CSS would need some work, but yeah this is the idea. Tim, does this patch work for you as far as focus / keyboard accessibility in Windows? ::: browser/themes/shared/devtools/webconsole.inc.css @@ +473,5 @@ > cursor: pointer; > + -moz-appearance: none; > +} > + > +button { Should change this selector to something like .message button Here are some properties I found that are applied to it when it is a link that we may want to include: -moz-user-focus: normal; -moz-user-input: enabled; @@ +484,5 @@ > + text-align: left; > + display: inline; > +} > + > +.cm-attribute { You won't actually need the .cm-attribute or button span selectors here. @@ +496,5 @@ > .elementNode:hover .open-inspector, > .open-inspector:hover { > background-position: -32px 0; > } > Also, there is at least one more selector in the webconsole css file that should be changed: .cm-s-mozilla a[class] that should be updated to .cm-s-mozilla button[class] (and the hover state as well)

Comment on attachment 8443740 [details] [diff] [review] Patch for 987373 Well, this is strange. The button is basically keyboard focusable (using the tab key), but using the enter key to open the pane doesn't work. Also, I noticed that there is no focus indicator on that link.

Here is an updated patch with the CSS changes that Brian mentions above. Brian, rather than replace `.cm-s-mozilla a[class]` with button[class], I merely added the button option, since in theory wont some a tags still exist if options.href has a value? This leads me to wonder if any of these need to be a tags, or if they can just all be buttons? Tim, does this patch fix the focus indicator issue you are referring to?

Comment on attachment 8443860 [details] [diff] [review] patch This fixes the focus indicator, but still doesn't open the sidebar when pressing the enter key after focus. Also, the inspect node button still doesn't have a focus indicator, you might want to use an outline as focus indicator. I also noticed a font issue on windows, which can be fixed using font: inherit;

Comment on attachment 8443860 [details] [diff] [review] patch Review of attachment 8443860 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/webconsole.inc.css @@ +458,5 @@ > text-decoration: none; > } > > .cm-s-mozilla a[class]:hover, > +.cm-s-mozilla a[class]:focus, Maybe we should just add a shared class to both tags (something like console-anchor). Then we could have any CSS apply to that class to simplify things and make sure it is consistent. I think we still need <a> tags for when the link is an actual URL - try console.log("http://mozilla.com")

(In reply to Tim Nguyen [:ntim] from comment #26) > Comment on attachment 8443860 [details] [diff] [review] > patch > > This fixes the focus indicator, but still doesn't open the sidebar when > pressing the enter key after focus. That's weird - maybe there is an event handler in JS looking for an <a> tag? Or maybe it is something to do with the button. Can you check if using a span instead of a button with the CSS properties below applied has a similar effect to the button tag without some of the baggage? -moz-appearance: none; -moz-user-focus: normal; -moz-user-input: enabled;

Comment on attachment 8443860 [details] [diff] [review] patch Review of attachment 8443860 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/webconsole.inc.css @@ +474,4 @@ > margin-left: 5px; > +} > + > +.message button { I would change this selector to the new shared CSS class, and also add font: inherit. And I guess we will need a :focus rule that adds some outline as well. Luckily if we use shared class it will be consistent for both links and the new tag

needinfo for Comment 28. I'm wondering if adding those CSS properties allow a <span> to have keyboard accessibility similar to a link in Windows.

Here's a fiddle testing out a few elements : http://jsfiddle.net/ntim/8N9am/ I think XUL might be causing some weird behaviour on the <button> element.

(In reply to Tim Nguyen [:ntim] from comment #31) > Here's a fiddle testing out a few elements : http://jsfiddle.net/ntim/8N9am/ > I think XUL might be causing some weird behaviour on the <button> element. On osx, the focus skips the <a> entirely, and I only get the alert on the <button> element. We surely don't want to set tabindex on each one of the elements in the console output so I guess we will have to stick with buttons. Based on Comment 26: > This fixes the focus indicator, but still doesn't open the sidebar when pressing the enter key after focus. This is weird, because it seems to work fine in your fiddle. I'm not sure what would be causing this to not work in the web console.

I used anchors because they are keyboard-focusable by default on windows and linux. On Mac OSX only inputs (and buttons?) are focusable by default. However, there's a setting you can enable: https://ets.berkeley.edu/help/how-do-i-turn-keyboard-focus-firefox-mac-os-x I would prefer that we dont use a button for the cases where there's no href - it is inconsistent with the other links. We can remove the href and set tabindex or some CSS property to make the anchor focusable.

The problem is that tabindex will ignore the click event, you wont be able to open the sidebar with the enter key.

is this bug still an issue, does it need to be worked on?

@brian Can I be assigned the bug? or is someone else already working on this? I can still re-create the issue from your screenshot from 2 years ago on a Macbook Pro. based of the comments I'm not really sure where the bug stands.

I don't think there's a clear direction yet in this bug. I left a comment in bug 1287389 to see if we might be able to reuse a component they are talking about adding for this in the inspector tabs: https://bugzilla.mozilla.org/show_bug.cgi?id=1287389

Yura, objects in the console right now are anchors (which was chosen for keyboard support originally). But it's causing a URL to show up in the status text when hovering it which we don't want. Any suggestions here? Switching to a button might be hard for styling and word wrapping but maybe a span with tabindex=0 and styled like a link. Will we be able to get equally good keyboard access and accessibility here if not using links?

Hi Brian, yes, this is likely semantically a link since we are navigating to a different tab/widget etc. So having a span with tabindex=0 is fine and it will be keyboard accessible. To add the semantics back all you need it role="link" attribute and the screen reader will treat the span as it would an anchor. https://www.w3.org/TR/wai-aria/roles#link

s/all you need it/all you need is

(In reply to Zach Tuttle from comment #36) > @brian > > Can I be assigned the bug? or is someone else already working on this? > > I can still re-create the issue from your screenshot from 2 years ago on a > Macbook Pro. > > based of the comments I'm not really sure where the bug stands. Zach, feel free to work on this. We should switch the <a> tag to be a span based on Comment 39. See devtools/client/webconsole/console-output.js and _openInspectorNode. This calls into the _anchor function, and what I'd suggest is we modify the _anchor function to render a "span" instead of an "a" if there's no href passed in as an option.

I think we can close this bug since it does not affect the new frontend. Do you see any reason not to Brian ?

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #42) > I think we can close this bug since it does not affect the new frontend. Do > you see any reason not to Brian ? Nice! We can close it then, especially since the icon / link text won't be visible in the Browser Console where we show the old frontend still.