1185808 - Warning Style sheets message can't be closed with long URIs

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P3)

Description

[Karl Dubost💡 :karlcow]

Attachment: Capture d’écran 2015-07-21 à 11.59.41.png

This happened on the site Theverge.com but it's related to an external resource. The URI is so long that it pushes away the "x" which is usually used to closed such a window.

A couple of ideas.
* Either an overflow hidden
* Or a wrapping of long lines.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The capture shows WebIDE, but I believe the error is from the Style Editor.

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Patrick Brosset <:pbro>]

Attachment: long-stylesheet-url-not-found.html

Some more details about this bug:

To reproduce it:
- open this attached test case HTML file in Firefox
- open the Style Editor panel
- notice that a warning message appears above devtools to say that the stylesheet could not be loaded
- now make your browser window smaller and smaller until the URL in the warning message doesn't fit anymore and devtools starts to overflow.

The problem: 
Not only can you not close the warning message because the X icon disappears to the right, but in fact the entire toolbox starts overflowing to the right of the window, so there are many other things you can't access anymore.

Solution:
We should definitely have an ellipsis text-overflow in place for long unbreakable URLs in this warning message.

Comment 4

[Patrick Brosset <:pbro>]

On no, you actually need to download the test case first and open it from your disk in Firefox. Serving it from Bugzilla means that a 404 is returned as content of the stylesheet and the problem isn't visible.

Comment 5

[Vincent]

Hi, I'm Vincent and would like to work on this (first) bug to ramp-up on the dev workflow and contribution process.
Can you assign it to me? thanks!

Comment 6

[Patrick Brosset <:pbro>]

Hey Vincent! Thanks for your interest! Let me assign this to you.

Also, here are a few pointers:
- our contribution docs are here, please go through them to install the dev environment and learn about how to submit code: http://docs.firefox-dev.tools/
- we have a public slack instance here: https://devtools-html-slack.herokuapp.com/ don't hesitate to join if you have any question at all
- to investigate this CSS issue, you will likely need to use the Browser Toolbox (the devtools for devtools). It needs a few settings to be enabled before you can use it, things should be explained here: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
- I suggest using the browser toolbox to find the HTML element(s) involved in showing the warning message, and then looking for them in the code to find out where to fix the issue. One nice search tool that might make it easier to find code than a text editor is searchfox: http://searchfox.org/

Don't hesitate to join Slack or ask questions here if any of this is unclear or if you need more help to get started.

Comment 7

[Vincent]

Attachment: improve_devtool_notificationbox_layout.patch

Comment 8

[Vincent]

Attachment: After patch screenshot

Comment 9

[Vincent]

Attachment: After patch screenshot

Comment 10

[Vincent]

Attachment: improve_devtool_notificationbox_layout.patch

New patch with eslint fixes.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 9021959 [details] [diff] [review]
improve_devtool_notificationbox_layout.patch

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

Thanks Vincent. This works great, and I think the code changes look good too. 
I've ran all the tests locally too, and because this is mostly CSS changes, I don't think we need to push this to TRY before merging it into an inbound branch.
So I'll mark this as checkin-needed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f9bded4e5b
Improve devtool notification box layout when overflowing. r=pbro

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out for mochitest failures on test_notification_box_03.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Csuccess%2Crunning%2Cpending%2Crunnable&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Ctest-linux64-asan%2Fopt-mochitest-chrome-1%2Cm%28c1%29&fromchange=16f9bded4e5bd8f317caf844a4a989ea15ef736a&tochange=4571b6b06612c509dd1268c07a4d35aba108e833&selectedJob=209779436

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=209779436&repo=mozilla-inbound&lineNumber=2912

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/4571b6b06612c509dd1268c07a4d35aba108e833

Comment 14

[Vincent]

The query for notification box buttons in devtools/client/shared/components/test/mochitest/test_notification_box_03.html needs to be updated with the new CSS class. I will do that later tonight.

Comment 15

[Patrick Brosset <:pbro>]

Oh you're right, thanks for finding the cause for this. Pushing to TRY would have been helpful after all :)
No big deal, as soon as you attach a new patch here, let's mark it as checkin-needed again.
Thanks Cosmin for backing this out.

Comment 16

[Vincent]

Attachment: improve_devtool_notificationbox_layout.patch

I updated the patch and fixed the test in devtools/client/shared/components/test/mochitest/test_notification_box_03.html.

I ran './mach test <path>' and mochitest tests were skipped. I had to run explicitly './mach mochitest <path>' to trigger the test failure.

Few questions:
 - Is the mochitest exclusion expected?
 - Can I push myself a change on try, before requesting reviews/checkins (mostly to avoid wasting people's time on broken changes)?

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Vincent from comment #16)
>  - Is the mochitest exclusion expected?
I'm not sure. mach test should act as a shortcut that automatically detects the type of test you're trying to run and use the corresponding command. I know it works on other types of tests, but it looks like it might not work with chrome mochitests.
>  - Can I push myself a change on try, before requesting reviews/checkins
> (mostly to avoid wasting people's time on broken changes)?
Yes you most certainly can! This requires mozilla commit access level 1 as described here: https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
For the first few patches, it's absolutely ok to ask the mentor/reviewer on a bug to push to TRY for you though, if you don't want to go through the trouble of getting the commit level access just yet.
In fact, I should have done it here for this bug, it's really quite quick to do, but I was really not expecting any failure.
Anyway, once you have the commit access, the easiest (at least for me) is to use the `mach try` command.
While running the command is easy, coming up with the right try syntax so the right tests get run is a bit more cumbersome.
Personally, I use this:

mach try -b do --artifact -p linux64,macosx64,win32,win64 -u xpcshell,mochitests -t none

(and I make sure my terminal remembers it so I don't have to ever type it again).
It runs tests on all 3 platforms, including mochitests and xpcshell tests which are the 2 main types of tests we use in devtools.
It also runs tests on both debug and optimized builds, to detect potential race conditions on slower builds.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 9022783 [details] [diff] [review]
improve_devtool_notificationbox_layout.patch

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

Thanks for addressing the test failure.

Comment 19

[Patrick Brosset <:pbro>]

Pushed to TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=822d2cbbeb7c2cd114ebcedd6f48cd61f90593a5

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48649b0c1220
Improve devtool notification box layout when overflowing. r=pbro

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/48649b0c1220

Comment 22

[Patrick Brosset <:pbro>]

And this landed in 65!
Thank you Vincent for contributing your first fix to the mozilla project, it's awesome!
For information, your code is now part of the mozilla-central repository at the address in comment 21. 
All current users of nightly (Firefox 65 currently) will be running your code as soon as the next nightly gets built.
On December 10th, your fix will move to the Beta branch, and I believe it will then get to all of our release users end of January.
Please feel free to ask either here or on Slack if you would like other similar (or different) bugs to work on next.

Comment 23

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly 2015-07-20 on Windows 10 x64.
I retested everything using latest Nightly 66.0a1 and beta 65.0b8 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64. The bug is not reproducing anymore.