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

VERIFIED FIXED in Firefox 65

Status

defect
P3
normal
VERIFIED FIXED
4 years ago
3 months ago

People

(Reporter: karlcow, Assigned: grosbouddha)

Tracking

(Blocks 1 bug)

40 Branch
Firefox 65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 verified, firefox66 verified)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
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

a year ago
Product: Firefox → DevTools
Blocks: 1493094
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

7 months 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

7 months ago
Attachment #9021933 - Flags: review?(pbrosset)
Assignee

Comment 8

7 months ago
Posted patch After patch screenshot (obsolete) — Splinter Review
Assignee

Comment 9

7 months ago
Attachment #9021934 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9021936 - Attachment is patch: false
Attachment #9021936 - Attachment mime type: text/plain → image/png
Assignee

Comment 10

7 months ago
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+

Comment 12

7 months 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

7 months 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

7 months ago
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+

Comment 20

7 months 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48649b0c1220
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months 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.

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