Closed Bug 696385 Opened 8 years ago Closed Last year

Remove underline for network requests on hover in webconsole

Categories

(DevTools :: Console, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: wes, Assigned: preeti, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=css])

User Story

**Steps to reproduce**
1. Go to : `data:text/html,<meta charset=utf8><script>fetch('http://example.com')</script>`
1. Open the console
1. Open the filter bar, and make sure the "xhr" button is enabled
1. reload the page
!. Hover the "http://example" text

**Actual results**

The text gets underlined and the cursor changes to pointer, which makes me think it is an actual link.
But when I click on it, it only expands the network detail panel.


---

We should remove the underline (and maybe the link), and change the tooltip to something like: "Click to show details for ${url}"

Attachments

(5 files, 1 obsolete file)

The links generated in the web console -- in particular by the net component -- are all black text underlined by black lines in a font which has tight vertical spacing.

This causes readability issues, and offers very little extra information.

I suggest changing the links to have text-decoration:underline only on hover.
Severity: normal → enhancement
Priority: -- → P3
triage. Filter on PINKISBEAUTIFUL
Component: Developer Tools → Developer Tools: Console
OS: Mac OS X → All
Hardware: x86 → All
Summary: Web console UX - hyperlinks → remove underline for network requests
Whiteboard: [mentor=msucan][lang=css]
Assigning to Affan as per in-person request (part of the IIT KGP MozSetup event)
Assignee: nobody → affanzafar07
Status: NEW → ASSIGNED
Attached patch Patch 1 (obsolete) — Splinter Review
I have fixed this
Attachment #8399039 - Flags: review?(mihai.sucan)
This is showing the console output with no underlines for network requests.
This screenshot shows the current console output, with no patch applied.
Comment on attachment 8399936 [details]
screenshot: patch applied

Darrin: we have an old bug here about removing underlines for network requests. This is a screenshot showing the console with a patch that removes the underlines.

In this bug you can also see attachment 8399938 [details] - the current state of the console output, with no patch applied.

The question I have for you: what should we do with the links? I'm not too happy with a simple underline removal. We have:

- generic links shown in red with underline - eg. [learn more].
- network requests and their status shown with black, underlined.
- object output which uses italic for links/clickable targets.
- and message source links on the right are blue.

This is messy. Can you please propose a unification of how we show links? Something consistent. I dont think we can show *all* links in one way - eg. object output is different conceptually, but I think we can do a better job than now.

Thanks a lot!
Attachment #8399936 - Flags: ui-review?(dhenein)
Comment on attachment 8399039 [details] [diff] [review]
Patch 1

Affan, thanks for your patch. This is looking good.

Please note this bug may not be a good first bug. We still have to decide on the desired UI. I have asked Darrin, our UX expert, for feedback on the current state of the console output. Let's wait for him, then we can finish the patch based on the feedback we get. Thanks!
Attachment #8399039 - Flags: review?(mihai.sucan)
Ok Mihai , I will wait for feedback.
Comment on attachment 8399039 [details] [diff] [review]
Patch 1

Review of attachment 8399039 [details] [diff] [review]:
-----------------------------------------------------------------

The patch works, but we should wait for UX decision.

Here are some comments about your patch meanwhile.

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +181,1 @@
>  }

Nit : new line needed here

@@ +181,3 @@
>  }
> +.message[category=network]:not(.navigation-marker) .url:hover{
> +	text-decoration:underline;

2 space indenting please.
Comment on attachment 8399039 [details] [diff] [review]
Patch 1

Review of attachment 8399039 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +181,5 @@
>  }
> +.message[category=network]:not(.navigation-marker) .url:hover{
> +	text-decoration:underline;
> +}
> +

Extra line here, please remove it
Mentor: mihai.sucan
Whiteboard: [mentor=msucan][lang=css] → [lang=css]
Comment on attachment 8399936 [details]
screenshot: patch applied

I personally don't mind the underlines, but would like a second opinion.
Attachment #8399936 - Flags: ui-review?(shorlander)
Attachment #8399936 - Flags: ui-review?(dhenein)
Attachment #8399936 - Flags: ui-review-
Mentor: mihai.sucan
Mentor: bgrinstead
Comment on attachment 8399936 [details]
screenshot: patch applied

I don't find this to be more readable, personally.
Attachment #8399936 - Flags: ui-review?(hholmes) → ui-review-
The new console front end seems to only display the underline on hover.
Depends on: enable-new-console
As Tom mentioned in comment 14, requests are not underlined by default anymore.
Though the underlining on hover indicates that the URL and request info at the right side are links, which they are not. Everywhere else in the DevTools underlining on hover actually means a switch to another panel. Therefore I believe the underline on hover should be removed, too (while keeping the pointer cursor to indicate that the log message can be expanded).

Furthermore, I mark this as good-first-bug, because that should be a relatively small change.
Instead of changing the CSS as indicated by the whiteboard info, I think it's better to turn the currently used <a> tags into <div>s and style them accordingly. Firstly, because those are not really links, and secondly, this avoids breaking UI of real links shown within the panel.

Brian, as mentor for this bug, do you agree?

Sebastian
Flags: needinfo?(bgrinstead)
Keywords: good-first-bug
(In reply to Sebastian Zartner [:sebo] from comment #15)
> As Tom mentioned in comment 14, requests are not underlined by default
> anymore.
> Though the underlining on hover indicates that the URL and request info at
> the right side are links, which they are not. Everywhere else in the
> DevTools underlining on hover actually means a switch to another panel.
> Therefore I believe the underline on hover should be removed, too (while
> keeping the pointer cursor to indicate that the log message can be expanded).

I'm not sure what we should do here. I'll forward the question to Nicolas and Victoria to see if they have a preference. I'm going to remove the good first bug keyword until we have a plan.

> Furthermore, I mark this as good-first-bug, because that should be a
> relatively small change.
> Instead of changing the CSS as indicated by the whiteboard info, I think
> it's better to turn the currently used <a> tags into <div>s and style them
> accordingly. Firstly, because those are not really links

I actually don't mind it being an <a> tag (with or without underline) if we allow ctrl+click on it open the URL in a new tab. Since we offer to open in a tab on a context menu I don't think that'd be a problem. Although, this UI is off by default so I don't feel really strongly about it.

> and secondly, this
> avoids breaking UI of real links shown within the panel.

What do you mean by that?
Assignee: affanzafar07 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(victoria)
Flags: needinfo?(nchevobbe)
Flags: needinfo?(bgrinstead)
Keywords: good-first-bug
Attached image network-hover-state.png
This is the hover state being discussed
I agree that this is confusing right now. The first step should be to remove the underline style on hover.
We can then have a separate bug to handle Ctrl/Cmd + click to open the link in a new tab. I filed Bug 1466040 for that
Mentor: bgrinstead → nchevobbe
Flags: needinfo?(nchevobbe)
Keywords: good-first-bug
Ah yes, it does make sense to remove the underline. Ideally the whole row would have some kind of hover state to show that it can be interacted with. I know we avoided adding a hover background color due to complication with the red/yellow background colors, but would be good to revisit this. E.g. it could just be the Inspector light blue hover when the row is white, and when the row is yellow or red, a slightly darker version of those colors. We could keep the blue border effect as a visual helper that appears on all rows including non-interactable ones.
Flags: needinfo?(victoria)
Product: Firefox → DevTools
hi I am Rupesh and I am new here and I want to contribute to mozilla but I dont know how to start contribute ,I got this bug but can not understand what to do, please help me to start contributing in mozilla.
Flags: needinfo?(nchevobbe)
Hello Rupesh, thanks a lot for helping us !

First, you need to setup the work environment. You can follow http://docs.firefox-dev.tools/getting-started/ to get to that point (make sure you work with artifact build, it's way faster).
Then, for this specific bug, you would need to do some modification to the CSS of network messages.
Since DevTools are mostly build using web technology, you can inspect and debug it using the Browser Toolbox https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox . 
In this case, you would open the regular webconsole, trigger a network request, open the browser toolbox, and with the"select element" picker, click on the network message to see which CSS class is applied.

Then, you can lookup in the mozilla repo code, either locally or via https://searchfox.org to look for the file you need to change.

Do doing that, you might get to the point where you find the class responsible for the underline: https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/themes/webconsole.css#469-472 .
Here we have 2 choices: either we change the CSS rule to not apply on network logs, or we don't render a link for this. I think the latter is better.

The webconsole is a react app, and we can render a <span> rather than a <a> by changing the dom.a by dom.div here https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/webconsole/components/message-types/NetworkEventMessage.js#109.

You should then be able to rebuild firefox (using ./mach build faster) and test the change.

At this point you can commit your change and push the patch up for review.

Let me know if you have any trouble during this process.
Flags: needinfo?(nchevobbe)
(In reply to Brian Grinstead [:bgrins] from comment #16)
> (In reply to Sebastian Zartner [:sebo] from comment #15)
> > and secondly, this
> > avoids breaking UI of real links shown within the panel.
> 
> What do you mean by that?

I meant, if it stays an <a> element and you change the related styles, you also change the styles for other parts of the UI, which are real links. It looks like Nicolas' comment 21 already considered this, so everything's fine. :-)

Sebastian
Hello,
I'm an absolute beginner and would like to take up this bug. Please guide me in proceeding with it.
 .
Hello Devika,

Rupesh might already be working on this one, but you might try anyway.
You can read Comment 21 to know how to proceed
User Story: (updated)
Summary: remove underline for network requests → Remove underline for network requests on hover in webconsole
Attachment #8399039 - Attachment is obsolete: true
Hello everyone! This is Preeti. I have just recently started learning JavaScript and would like to work on this bug, in case no one is working on this. I have already setup the environment. But I have a doubt:

As mentioned in comment #21(In reply to Nicolas Chevobbe [:nchevobbe] from comment #21)

> Do doing that, you might get to the point where you find the class
> responsible for the underline:
> https://searchfox.org/mozilla-central/rev/
> 6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/themes/webconsole.
> css#469-472 .
> Here we have 2 choices: either we change the CSS rule to not apply on
> network logs, or we don't render a link for this. I think the latter is
> better.
Could you please elaborate on how not to render the link?
Can we consider changing line number 471 of mozilla-central/devtools/client/themes/webconsole.css from
text-decoration: underline;
to
text-decoration: none;
Flags: needinfo?(nchevobbe)
(In reply to Preeti from comment #25)
> Hello everyone! This is Preeti. I have just recently started learning
> JavaScript and would like to work on this bug, in case no one is working on
> this. I have already setup the environment. But I have a doubt:
> 
> As mentioned in comment #21(In reply to Nicolas Chevobbe [:nchevobbe] from
> comment #21)
> 
> > Do doing that, you might get to the point where you find the class
> > responsible for the underline:
> > https://searchfox.org/mozilla-central/rev/
> > 6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/themes/webconsole.
> > css#469-472 .
> > Here we have 2 choices: either we change the CSS rule to not apply on
> > network logs, or we don't render a link for this. I think the latter is
> > better.
> Could you please elaborate on how not to render the link?
> Can we consider changing line number 471 of
> mozilla-central/devtools/client/themes/webconsole.css from
> text-decoration: underline;
> to
> text-decoration: none;

Hello Preeti !
So the CSS fix you suggest would be a way of getting rid of the underline. But, from a semantic standpoint, it would be better if we wouldn't render a `<a>` element but a `<span>` instead. This would make sense since it's not a link (there's no href on it), only a text displaying an URL.

The webconsole is a react app, and we can render a <span> rather than a <a> by changing the `dom.a` by `dom.span` here https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/webconsole/components/message-types/NetworkEventMessage.js#109.

Then, there will be probably some test to change, and some styling to do.

Also, I'm not assigning this bug right now since a few people asked to work on it (but I don't know if they are).
Flags: needinfo?(nchevobbe)
> Hello Preeti !
> So the CSS fix you suggest would be a way of getting rid of the underline.
> But, from a semantic standpoint, it would be better if we wouldn't render a
> `<a>` element but a `<span>` instead. This would make sense since it's not a
> link (there's no href on it), only a text displaying an URL.
> 
> The webconsole is a react app, and we can render a <span> rather than a <a>
> by changing the `dom.a` by `dom.span` here
> https://searchfox.org/mozilla-central/rev/
> 6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/webconsole/
> components/message-types/NetworkEventMessage.js#109.
> 
> Then, there will be probably some test to change, and some styling to do.

Thank you very much for your explanation!
> 
> Also, I'm not assigning this bug right now since a few people asked to work
> on it (but I don't know if they are).

In case you get to know soon that this bug is free to work on, please do let me know. Thanks in advance!
Preeti, you can start working on it, I'll assign the bug to the next person that upload a patch here :)
Even if someone ends up posting something before you, that should be a good practice to navigate the codebase, build and run Firefox, …
No more underline, that's great ! Thanks Preeti.
We should do a CSS fix though: with your patch, if we hover the URL, you get a "text selection" cursor. It would be nice if we could set the `cursor` to `default` since it's clickable.

Also, I pushed your change to TRY, which means will run all the test we have to make sure this does not break anything: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a639a3870a25df9d089fa2aebd68fc7bc2a09d4

So if everything is fine, with the CSS change applied, we can land this patch :)
Assignee: nobody → preetimukherjee98
Status: NEW → ASSIGNED
TRY looks good ! So let's make this small CSS change and then we can land this patch 🎉
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #31)
> TRY looks good ! So let's make this small CSS change and then we can land
> this patch 🎉

Thank you Nicolas for your kind appreciation. But for the CSS patch, I could not figure out which file to modify. Would changing the following file in link help?Thanks in advance!

https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/devtools/client/themes/webconsole.css#483
Preeti, 
You can use the Browser Toolbox to inspect the DevTools (see https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox).
It would be similar to what youy can do with a web page, so you'll be able to see the rule applied to the element, and add/change properties to see how the UI react to that.
To answer your question, yes, the change will likely happen in webconsole.css
Comment on attachment 9003561 [details]
Bug 696385 - change dom.a to dom.span in mozilla-central/devtools/client/webconsole/components/message-types/NetworkEventMessage.js#109; r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9003561 - Flags: review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aca1ad7c8442
change dom.a to dom.span in mozilla-central/devtools/client/webconsole/components/message-types/NetworkEventMessage.js#109; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/aca1ad7c8442
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.