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

RESOLVED FIXED in Firefox 65

Status

P3
normal
RESOLVED FIXED
3 years ago
8 days ago

People

(Reporter: karlcow, Assigned: grosbouddha)

Tracking

(Blocks: 1 bug)

40 Branch
Firefox 65

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8636378 [details]
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.
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

Updated

5 months ago
Product: Firefox → DevTools

Updated

2 months ago
Blocks: 1493094
Created attachment 9019953 [details]
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.
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.
(Assignee)

Comment 5

15 days ago
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: 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.
Assignee: nobody → grosbouddha
Status: NEW → ASSIGNED
(Assignee)

Comment 7

13 days ago
Created attachment 9021933 [details] [diff] [review]
improve_devtool_notificationbox_layout.patch
Attachment #9021933 - Flags: review?(pbrosset)
(Assignee)

Comment 8

13 days ago
Created attachment 9021934 [details] [diff] [review]
After patch screenshot
(Assignee)

Comment 9

13 days ago
Created attachment 9021936 [details]
After patch screenshot
Attachment #9021934 - Attachment is obsolete: true
(Assignee)

Updated

13 days ago
Attachment #9021936 - Attachment is patch: false
Attachment #9021936 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 10

12 days ago
Created attachment 9021959 [details] [diff] [review]
improve_devtool_notificationbox_layout.patch

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]
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.
Attachment #9021959 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed

Comment 12

9 days ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f9bded4e5b
Improve devtool notification box layout when overflowing. r=pbro
Keywords: checkin-needed
(Assignee)

Comment 14

9 days ago
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.
(Assignee)

Comment 16

8 days ago
Created attachment 9022783 [details] [diff] [review]
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)?
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: 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 on attachment 9022783 [details] [diff] [review]
improve_devtool_notificationbox_layout.patch

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

Thanks for addressing the test failure.
Attachment #9022783 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed

Comment 20

8 days ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48649b0c1220
Improve devtool notification box layout when overflowing. r=pbro
Keywords: checkin-needed

Comment 21

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48649b0c1220
Status: ASSIGNED → RESOLVED
Last Resolved: 8 days ago
status-firefox65: --- → fixed
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.
You need to log in before you can comment on or make changes to this bug.