Elements bar can show many text artifacts after different element selected

RESOLVED FIXED in Firefox 52

Status

P1
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: aryx, Assigned: pbro)

Tracking

({regression})

47 Branch
Firefox 53
regression

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52- fixed, firefox53- fixed)

Details

Attachments

(6 attachments)

Created attachment 8814214 [details]
screenshot of issue

Firefox 53.0a1 and 52.0a2 20161124 on Windows 8.1

If the user selects a different element with the inspector, the bar with the element hierarchy can show text artifacts.

Steps to reproduce:
1. Load http://gridbyexample.com/examples/example4/
2. Open Inspector (<body> should be selected).
3. Select one of the six boxes.
(Assignee)

Comment 1

2 years ago
This is most probably a Windows 10 specific bug. :jdescottes tested on Windows 7 and :zer0 on Mac and neither of them could reproduced. I did reproduce on Windows 10 though.
I tested on 53.

We should take a look as soon as possible since it's also in 52, so this might require an uplift.

This definitely looks like a rendering issue. 
Cameron: don't know who to ping on platform side exactly for this, but it would be great to have someone working on the platform investigate this.
Flags: needinfo?(cam)
Keywords: regression, regressionwindow-wanted
Priority: -- → P1

Comment 2

2 years ago
I can only reproduce the text artifact on Win10 if HWA is disabled.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=717d85fc2046149b2a3443ffcc7aff04e119b0f7&tochange=729307f6ac594f76d1de7d7697ca5c3880135de3

Regressed by:729307f6ac59	Andrew Osmond — Bug 1007702 - Enable skia on nighly for unaccelerated windows. r=lsalzman

Updated

2 years ago
Blocks: 1320392
Milan, can you find someone to look at this?
Flags: needinfo?(cam) → needinfo?(milan)

Updated

2 years ago
Keywords: regressionwindow-wanted
I don't see the problems on unaccelerated Windows 10.  Lee, Mason, can either of you reproduce this?  Alice/Sebastian, can we have your about:support (graphics section)?
Flags: needinfo?(milan)
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(alice0775)
Flags: needinfo?(mchang)
Flags: needinfo?(lsalzman)

Comment 5

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #4)
> I don't see the problems on unaccelerated Windows 10.  Lee, Mason, can
> either of you reproduce this?  Alice/Sebastian, can we have your
> about:support (graphics section)?

see attachment 8814505 [details]
Flags: needinfo?(alice0775)
Assignee: nobody → mchang
Flags: needinfo?(mchang)
Created attachment 8815038 [details]
graphics info and more

I'm on Windows 8.1 64-bit and this is the requested graphics info (and more from about:support).
Flags: needinfo?(aryx.bugmail)
Created attachment 8815117 [details]
Movie of glitch

I don't think this is a skia bug. I can reproduce this with D2D. STR:

1) start with steps in comment 0
2) Once you selected an element with the bad text, don't touch anything for a couple of seconds.
3) Text appears properly

With D2D, instead of getting garbled text, I'm getting empty text or partial text instead. If I use the keyboard to inspect another div, then use the keyboard to go back to the original, I can reliably reproduce this.
Flags: needinfo?(pbrosset)
Flags: needinfo?(aryx.bugmail)
Sorry, I should've said I think this is a devtools bug instead.
I can reproduce this all the way back to Firefox 47.0.1.
Flags: needinfo?(lsalzman)
Yes, I can also reproduce it in 48+ (with the DOM navigation bar at the bottom).
Flags: needinfo?(aryx.bugmail)
(Assignee)

Comment 11

2 years ago
(In reply to Mason Chang [:mchang] from comment #7)
> Created attachment 8815117 [details]
> Movie of glitch
> 
> I don't think this is a skia bug. I can reproduce this with D2D. STR:
> 
> 1) start with steps in comment 0
> 2) Once you selected an element with the bad text, don't touch anything for
> a couple of seconds.
> 3) Text appears properly
> 
> With D2D, instead of getting garbled text, I'm getting empty text or partial
> text instead. If I use the keyboard to inspect another div, then use the
> keyboard to go back to the original, I can reliably reproduce this.
Aren't these 2 different bugs though? With skia we get garbled text. With d2d we get partial text.

(In reply to Mason Chang [:mchang] from comment #8)
> Sorry, I should've said I think this is a devtools bug instead.
So maybe something devtools uses causes that bug, but this is a text rendering bug, right? Devtools only uses HTML and CSS to render its UI, so I don't really get that point.
I will try and investigate a little bit what in devtools CSS causes this rendering issue to appear.

My gut feeling tells me that perhaps we're using a combination of HTML and CSS that causes d2d to render partial text, and skia to render garbled text. But wouldn't that mean that the fix should be in these libraries ultimately?
Flags: needinfo?(pbrosset)
(Assignee)

Comment 12

2 years ago
I'm trying to replicate this outside of devtools: http://jsbin.com/sutumimedu/edit?html,css,output
So far no luck. Maybe I need to import more of the structure of the devtools UI to this test case.
For now, I've only extracted the breadcrumbs area.

I did find what was causing the garbled though: it's a combination of us using a -moz-element CSS background and using scrollIntoView to make the breadcrumbs scroll to the end when a new node is selected in the inspector.

If I remove the -moz-element background, the problem goes away.
If I keep it but remove the scrollIntoView, the problem also goes away.

Hopefully this can help someone investigate further. I don't know what else to do from here.
(In reply to Patrick Brosset <:pbro> from comment #12)
> I'm trying to replicate this outside of devtools:
> http://jsbin.com/sutumimedu/edit?html,css,output
> So far no luck. Maybe I need to import more of the structure of the devtools
> UI to this test case.
> For now, I've only extracted the breadcrumbs area.
> 
> I did find what was causing the garbled though: it's a combination of us
> using a -moz-element CSS background and using scrollIntoView to make the
> breadcrumbs scroll to the end when a new node is selected in the inspector.
> 
> If I remove the -moz-element background, the problem goes away.
> If I keep it but remove the scrollIntoView, the problem also goes away.
> 
> Hopefully this can help someone investigate further. I don't know what else
> to do from here.

What's odd to me is that after a few seconds, the text shows up correctly. Do you know what's happening between the time someone selects a new node and when the text shows up correctly? Is there a timer doing something that isn't happening during the initial selection?

A smaller test case would be quite helpful as well.
Flags: needinfo?(pbrosset)
Version: unspecified → 47 Branch

Comment 14

2 years ago
Created attachment 8822920 [details]
div.html (testcase)

It's easy to reproduce, you just need en element with a very long path (with many class/id), you can use my testcase.
This issue is getting worse with the activation of GPU process in bug 1314133 + e10s, and it's probably related to the regressing bug 1320392.

In Nightly:
1) e10s on + HWA on + GPU process on: bug 1320182
2) e10s off + HWA on + GPU process on: bug 1320392
3) e10s on/off + HWA on + GPU process off: bug 1320392
4) e10s on + HWA off + GPU process off: bug 1320182
5) e10s off + HWA off + GPU process off: no bug
6) e10s on + HWA off + GPU process on: bug 1320182
7) e10s off + HWA off + GPU process on: no bug

Updated

2 years ago
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
(Assignee)

Comment 15

2 years ago
Created attachment 8823246 [details]
breadcrumbs-glitch-on-resize.gif

(In reply to Mason Chang PTO 1/3/17 [:mchang] from comment #13)
> What's odd to me is that after a few seconds, the text shows up correctly.
Interesting, I see this too, but only some of the times.
> Do you know what's happening between the time someone selects a new node and
> when the text shows up correctly? Is there a timer doing something that
> isn't happening during the initial selection?
Unfortunately there is nothing inside the breadcrumbs widget code [1] that does something after the initial selection.
There is a timer in this code [2] but it only runs if you click on the scroll buttons to the left and right of the widget. And there's another one [3] that runs after 0ms to scroll the widget all the way to the end so the last crumb is visible when a node gets selected. So that can't be it because when I see the text reappear, it does so only after a few seconds.

One more thing I noticed: if you drag the splitter between devtools and the tab content area, then that makes the text in  the breadcrumbs appear and disappear. If you drag it slowly, you can see the text being re-rendered, and its vertical position shifting a bit.
I've attached a GIF recording to demonstrate this.

I'm sorry I wish I had an isolated test case to reproduce this, but I didn't manage to create one. It seems like a lot of things from the inspector's CSS are at play here.
As I said in comment 12, the -moz-element background seems to be the biggest culprit here. Maybe we should investigate getting rid of it.

@bgrins: do you remember why we used this type of background in the first place. Do you think there's a way we could replace it with another type of background? There might be a way to do the same graphic using a couple of linear-gradients.

[1] http://searchfox.org/mozilla-central/source/devtools/client/inspector/breadcrumbs.js
[2] http://searchfox.org/mozilla-central/source/devtools/client/inspector/breadcrumbs.js#116
[3] http://searchfox.org/mozilla-central/source/devtools/client/inspector/breadcrumbs.js#920
Flags: needinfo?(pbrosset) → needinfo?(bgrinstead)
IIRC we are using moz-element to handle the arrow shape that goes between crumbs without needing to modify the markup. I'd be in favor of simplifying the UI here to get rid of this 'notched out' background entirely, which would make it easy to get rid of the moz-element.  There are some mockups floating around that do this - see the bottom of https://projects.invisionapp.com/share/GN527IGMZ#/screens/179720294.
Flags: needinfo?(bgrinstead)
It sounds like this is not a new regression, so not tracking for 52/53.  Unless for some other reason this has become much worse now?
tracking-firefox52: ? → -
tracking-firefox53: ? → -

Comment 18

2 years ago
(In reply to Julien Cristau [:jcristau] from comment #17)
> It sounds like this is not a new regression, so not tracking for 52/53. 
> Unless for some other reason this has become much worse now?

I disagree with that, read my comment #14. Enabling GPU processing has intensified the issue in 52/53. The initial regression (bug 1320392) is until 49. So it's a recent issue.
(Assignee)

Comment 19

2 years ago
I've spent some time on this refactoring the devtools code to avoid using -moz-element. I think I managed to come up with some new CSS to avoids the problem.
So, while this isn't addressing the underlying rendering issue, I feel like it's a good simplification of our CSS that avoids a crazy problem unlikely to be fixed because of the complexity of the test case.

I'll upload a devtools patch for review soon and we can then decide of the best course of action.
Comment hidden (mozreview-request)
(Assignee)

Comment 21

2 years ago
The patch I attached just now replaces the background image with a set of linear-gradients.
I tested this with all 3 devtools themes and with RTL. Looks like it's working for me on Windows. I'd appreciate some testing on Mac if possible.

Comment 22

2 years ago
mozreview-review
Comment on attachment 8825380 [details]
Bug 1320182 - Remove -moz-element background from breadcrumbs and use linear-gradient instead;

https://reviewboard.mozilla.org/r/103552/#review104210

Great patch! Tested on OSX and Windows 7, works perfectly. 
Later we could simplify the shape of the separator to avoid having to maintain this css code.

A small issue: the old debugger is using the BreadcrumbsWidget.jsm which also uses the "breadcrumb-separator-container".
The background of the debugger is different from the one used in the inspector, so the new style looks a bit off in there.
I know the old debugger is going away, but people might still enable it if they miss features in the new one, and if we aggressively uplift this, it might even beta, where the new debugger is not yet preffed on I think.

::: devtools/client/themes/widgets.css:218
(Diff revision 1)
>  }
>  
> -.breadcrumbs-widget-item:not([checked]) {
> -  background: -moz-element(#breadcrumb-separator-normal) no-repeat center left;
> +.breadcrumbs-widget-item::before {
> +  content: "";
> +  position: absolute;
> +  top: 0;

When the dotted focus outline is displayed, it gets hidden by each separator. Could we set top: 1px and height: 22px; here to avoid that?

Comment 23

2 years ago
mozreview-review
Comment on attachment 8825380 [details]
Bug 1320182 - Remove -moz-element background from breadcrumbs and use linear-gradient instead;

https://reviewboard.mozilla.org/r/103554/#review104218

Hmmm ok, quite confused by this new mozreview layout, I left a global review and couldn't set r+ there.
Attachment #8825380 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 24

2 years ago
(In reply to Julian Descottes [:jdescottes] from comment #22)
> Later we could simplify the shape of the separator to avoid having to
> maintain this css code.
Totally agree here. I just wanted to make exactly the same shape to avoid having to think about UI here.
But we could even get rid of the separator altogether in another bug. Brian mentioned this in comment 16 and linked to some mockups that we could use. Let's try to land this quickly and then work on simplifying the breadcrumbs design later.
(Assignee)

Comment 25

2 years ago
(In reply to Julian Descottes [:jdescottes] from comment #22)
> A small issue: the old debugger is using the BreadcrumbsWidget.jsm which
> also uses the "breadcrumb-separator-container".
> The background of the debugger is different from the one used in the
> inspector, so the new style looks a bit off in there.
> I know the old debugger is going away, but people might still enable it if
> they miss features in the new one, and if we aggressively uplift this, it
> might even beta, where the new debugger is not yet preffed on I think.
Fixed. Thanks. I also removed the separator image we were using because it's not used anymore now.
> When the dotted focus outline is displayed, it gets hidden by each
> separator. Could we set top: 1px and height: 22px; here to avoid that?
Yeah, that seems to work. If you look really closely, it doesn't look at as good, but knowing that we will change the design of the breadcrumbs soon, and that it's more important that the focus outline is shown correctly, we can live with this.
Comment hidden (mozreview-request)

Comment 28

2 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c06c9ae6042e
Remove -moz-element background from breadcrumbs and use linear-gradient instead; r=jdescottes

Updated

2 years ago
Duplicate of this bug: 1320392
(Assignee)

Updated

2 years ago
Assignee: mchang → pbrosset
Status: NEW → ASSIGNED

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c06c9ae6042e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

2 years ago
No longer blocks: 1320392
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(pbrosset)
(Assignee)

Comment 32

2 years ago
Comment on attachment 8825380 [details]
Bug 1320182 - Remove -moz-element background from breadcrumbs and use linear-gradient instead;

Approval Request Comment
[Feature/Bug causing the regression]: regression caused by bug 1007702 as comment 2 says.
[User impact if declined]: If we decline this, then Windows (10 only?) users are going to see weird text rendering glitches in the inspector panel of devtools (in the breadcrumbs area only).
[Is this code covered by automated tests?]: No. This is some sort of a platform rendering issue that was solved by changing the breadcrumbs' CSS. There are no devtools automated tests that can check the way text is rendered.
[Has the fix been verified in Nightly?]: Manually yes. Not by QE though.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, that would be good.
STR:
- open http://jsbin.com/?html,output
- open the inspector
- click on the icon to start selecting an element from the page
- select any piece of code from the HTML pane inside the jsbin page
Basically, as soon as the breadcrumbs widget auto-scrolls to the right to reveal the selected node, then the text gets weird/hidden.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: I refactored the CSS of the breadcrumbs widget. It's not impacting the browser, only devtools. If there was a bug in my code changes, then it would impact only the breadcrumbs widget. But there are many automated tests on this widget, so we can be sure that I didn't break its logic at least.
Flags: needinfo?(pbrosset)
Attachment #8825380 - Flags: approval-mozilla-aurora?
Comment on attachment 8825380 [details]
Bug 1320182 - Remove -moz-element background from breadcrumbs and use linear-gradient instead;

style change for devtools inspector breadcrumbs, aurora52+
Attachment #8825380 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 34

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d75736de7c46
status-firefox52: affected → fixed

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.