Closed Bug 1185808 Opened 9 years ago Closed 5 years ago

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


(DevTools :: Style Editor, defect, P3)

40 Branch


(firefox65 verified, firefox66 verified)

Firefox 65
Tracking Status
firefox65 --- verified
firefox66 --- verified


(Reporter: karlcow, Assigned: grosbouddha)


(Blocks 1 open bug)



(4 files, 3 obsolete files)

This happened on the site 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.
The capture shows WebIDE, but I believe the error is from the Style Editor.
Component: Developer Tools → Developer Tools: Style Editor
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Product: Firefox → DevTools
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.

We should definitely have an ellipsis text-overflow in place for long unbreakable URLs in this warning message.
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.
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!
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:
- we have a public slack instance here: 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:
- 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:

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.
Assignee: nobody → grosbouddha
Attachment #9021933 - Flags: review?(pbrosset)
Attached patch After patch screenshot (obsolete) — Splinter Review
Attached image After patch screenshot
Attachment #9021934 - Attachment is obsolete: true
Attachment #9021936 - Attachment is patch: false
Attachment #9021936 - Attachment mime type: text/plain → image/png
New patch with eslint fixes.
Attachment #9021933 - Attachment is obsolete: true
Attachment #9021933 - Flags: review?(pbrosset)
Attachment #9021959 - Flags: review?(pbrosset)
Comment on attachment 9021959 [details] [diff] [review]

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.
Attachment #9021959 - Flags: review?(pbrosset) → review+
Pushed by
Improve devtool notification box layout when overflowing. r=pbro
Keywords: checkin-needed
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.
Flags: needinfo?(grosbouddha)
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.
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)?
Attachment #9021959 - Attachment is obsolete: true
Attachment #9022783 - Flags: review?(pbrosset)
(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:
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 on attachment 9022783 [details] [diff] [review]

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

Thanks for addressing the test failure.
Attachment #9022783 - Flags: review?(pbrosset) → review+
Pushed by
Improve devtool notification box layout when overflowing. r=pbro
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
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.
Flags: qe-verify+

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.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.