987373 - When hovering the 'click to select node' icon in console output, a chrome:/// URL shows up as the link preview

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Brian Grinstead [:bgrins]]

(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#

Comment 2

[Mihai Sucan [:msucan]]

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

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[alexharris6]

I can look into this.

Comment 5

[Brian Grinstead [:bgrins]]

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

Comment 6

[Tim Nguyen :ntim]

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 ?)

Comment 7

[Brian Grinstead [:bgrins]]

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

Comment 8

[alexharris6]

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

Comment 9

[Brian Grinstead [:bgrins]]

(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);
  }

Comment 10

[Tim Nguyen :ntim]

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

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[alexharris6]

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`?

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Brian Grinstead [:bgrins]]

(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

Comment 15

[Brian Grinstead [:bgrins]]

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?

Comment 16

[alexharris6]

Attachment: Patch for 987373

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 17

[Brian Grinstead [:bgrins]]

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 18

[Tim Nguyen :ntim]

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.

Comment 19

[Brian Grinstead [:bgrins]]

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

Comment 20

[alexharris6]

Attachment: Screen Shot 2014-06-19 at 10.00.50 PM.png

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.

Comment 21

[Brian Grinstead [:bgrins]]

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

Comment 22

[alexharris6]

Attachment: Patch for 987373

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 23

[Brian Grinstead [:bgrins]]

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 24

[Tim Nguyen :ntim]

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.

Comment 25

[alexharris6]

Attachment: patch

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 26

[Tim Nguyen :ntim]

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 27

[Brian Grinstead [:bgrins]]

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")

Comment 28

[Brian Grinstead [:bgrins]]

(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 29

[Brian Grinstead [:bgrins]]

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

Comment 30

[Brian Grinstead [:bgrins]]

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.

Comment 31

[Tim Nguyen :ntim]

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.

Comment 32

[Brian Grinstead [:bgrins]]

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

Comment 33

[Mihai Sucan [:msucan]]

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.

Comment 34

[Tim Nguyen :ntim]

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

Comment 35

[Zach Tuttle]

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

Comment 36

[Zach Tuttle]

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

Comment 37

[Brian Grinstead [:bgrins]]

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

Comment 38

[Brian Grinstead [:bgrins]]

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?

Comment 39

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 40

[Yura Zenevich [:yzen] (please use "needinfo")]

s/all you need it/all you need is

Comment 41

[Brian Grinstead [:bgrins]]

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

Comment 42

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 43

[Brian Grinstead [:bgrins]]

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