Closed Bug 1276467 Opened 4 years ago Closed 4 years ago

Style Editor caret looks weird (and text is duplicated on jsfiddle.net)

Categories

(Core :: Layout, defect, P1)

48 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox49 + verified
firefox50 --- verified

People

(Reporter: nchevobbe, Assigned: tnikkel)

References

Details

(Keywords: regression)

Attachments

(8 files, 1 obsolete file)

Attached image stylesheet.gif
STR: 
1. Open `data:text/html,<p>hello` url
2. Open the devtools
3. Go to the style editor tab
4. Click on the "New" button in the left panel
5. The stylesheet editor should be focused

Expected Result:
Caret looks like a regular blinking caret

Actual Result:
Caret has some sort of blue outline around it (see gif attached)

IIRC , there were some works on `focus`styles on the devtools, might be a side effect.
FYI, I see the same problem when "editing as html" in the markup view.

Marking this as a P1 as I think it should be fixed quickly, but feel free to change it if you disagree.
Reproduced on Mac and Windows (both with today's fx-team build). So this isn't a Mac only issue, however it does look different on Windows. I'll attach a GIF next.
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified
On Mac too, the blue highlight around the cursor blinks, but at a slightly different rate than the cursor itself, it's very weird. Also, if you add multiple cursors (cmd+click), only the last one has the highlight around it, not the other ones.
Couldn't get a good regression window with mozregression. The issue kept appearing sort of randomly.
The problem is that it sometimes took time for the blue outline to appear around the cursor. Sometimes it would appear immediately, sometimes after a while, and sometimes wouldn't. That made it really hard to determine if a build was good or bad, and hence I couldn't find the regression window.

What I did notice though is that at some point in time, we started getting 2 cursors in the style-editor: 2 vertical lines blinking side by side at different rates. 
Attached is a GIF that shows this.
(In reply to Patrick Brosset <:pbro> from comment #4)
> What I did notice though is that at some point in time, we started getting 2
> cursors in the style-editor: 2 vertical lines blinking side by side at
> different rates. 
> Attached is a GIF that shows this.
For the double cursor, here's a regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cc9051869d1a9078a821da5d1639299f9e836e0e&tochange=706b5b989592665258da854f186de3829a21769d
Attached image codemirror.gif
So, this does not look like a devtools issue, I was able to reproduce it on the codemirror website (https://codemirror.net/) too.
Thanks for confirming Nicolas.
I was also able to reproduce the double cursor (mentioned in comment 4) on Mac. And after some searching on bugzilla, concluded that this was bug 1273346.

What's interesting is that it is the second cursor that receives the blue outline around it on Mac. And that if you scroll, you'll see that second cursor stay fixed while the other ones scrolls with the content.
See Also: → 1273346
Thanks for the investigation, I was able to reproduce this problem.
Blocks: 1265237
Attached file reduced testcase
Assignee: nobody → tnikkel
Component: Developer Tools: Style Editor → Layout
Product: Firefox → Core
The problem here is that in nsDisplayListBuilder::EnterPresShell we call MarkFrameForDisplay on the frame containing the caret, which adds the "force descend" bit to the caret frame and all ancestors. The fix for bug 1265237 was depending on the only user of the "force descend" bit to be out of flow frames in the following way: if we entered an out of flow frame without stored out of flow data and the "force descend" bit set then we cleared the clip because we assumed that whatever content that was visible would have saved out of flow data and could restore the clip to the correct value. The other two users of the "force descend" bit (caret frame and preserve 3d) don't store out of flow data, so this assumption is incorrect.

However, the dirty rect is still empty so there is another reason that the textarea gets drawn. That reason is nsDisplayItem::RecomputeVisibility. It uses the intersects the visible region with the clipped bounds of the item and overwrites what is stored in mVisibleRect with that (mVisibleRect was empty on the items in question before RecomputeVisibility). This is easy enough to fix (assuming I'm not missing an obvious reason why we don't do this already and it isn't masking other visible rect bugs), and sufficient to fix this bug.

To fix the first problem the only thing I can think of is to split the "force descend" bit into two: one bit for out of flow processing (which saves out of flow data), and one bit for caret/preserve3d.
Attached patch dontexpandvisrect (obsolete) — Splinter Review
Attachment #8758604 - Flags: review?(matt.woodrow)
Thanks Timothy. For info, this seems to be happening on 48 too.
Version: 49 Branch → 48 Branch
Comment on attachment 8758604 [details] [diff] [review]
dontexpandvisrect

This is quite orange on try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=130e0fa1f3a5

So I guess I'll use another frame state bit and implement the other fix.
Duplicate of this bug: 1277251
Carrying over the tracking flags from the duplicate bug. We'll track this for 48/49.
Comment on attachment 8758604 [details] [diff] [review]
dontexpandvisrect

Removing review request since we're waiting on the new patch.
Attachment #8758604 - Flags: review?(matt.woodrow)
Actually I don't think clearing the clip can work (even only in the right cases) if display items with empty visible rect get drawn. We could create display items for any content after we clear the clip but before we enter the placeholder that has the valid clip stored for it. And they could all get drawn.

So if we want to clear the clip, we need to make sure empty visible rect display items don't draw.

Try only fails one test if we refuse to expand an empty visiblerect:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acba2f664de

A test I wrote/fixed for xul scrollables. I'll debug it to see why it fails.
The test fails because we call RecomputeVisibility on an item twice, once for each part of a rotated buffer. The first time it becomes empty, and then when it is visible for the second part it stays not visible because of my patch.

So I guess I'll need a bool on display item recording if it was invisible when it was created.
[updating summary to mention the worse symptom here -- duplicated, unreadable text on e.g. jsfiddle, as shown in this screencast: attachment 8763131 [details]]

It would be awesome if we could avoid shipping this regression in Firefox 48 -- it makes jsfiddle's interface pretty near unusable.
Keywords: regression
Summary: Style Editor caret looks weird → Style Editor caret looks weird (and text is duplicated on jsfiddle.net)
Comment on attachment 8763258 [details] [diff] [review]
hideinvisitems

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

::: layout/generic/nsFrame.cpp
@@ +2804,5 @@
>    } else if (GetStateBits() & NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO &&
>               isPlaceholder) {
> +    NS_ASSERTION(dirty.IsEmpty(), "should have empty dirty rect");
> +    // Every item we build from now until we descent into an out of flow that
> +    // does have saved out of flow data should be invisible.

Comment that this state also gets reset when we exit this scope
Attachment #8763258 - Flags: review?(matt.woodrow) → review+
Looking good, want to land this? (And then request uplift as far as you think it is safe to go) Thanks!
Flags: needinfo?(tnikkel)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> Looking good, want to land this? (And then request uplift as far as you
> think it is safe to go) Thanks!

Still working on some try failures.
Flags: needinfo?(tnikkel)
Duplicate of this bug: 1279748
Attached patch further fixesSplinter Review
We fail layout/base/tests/test_preserve3d_sorting_hit_testing2.html?q=test_preserve3d_sorting_hit_testing2.html because we trigger the asserts that the patch added. This is because in nsIFrame::FinishAndStoreOverflow the overflow rect of preserve3d children is set to the empty rect. So in nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay the intersection test results in empty and we don't save oof data. But preserve3d marks the force descend bit on the whole preserve3d subtree. nsFrame::BuildDisplayListForStackingContext gets the proper dirty rect from the builder via GetPreserve3DDirtyRect.

Not sure what the best fix is for this. It seems wrong that MarkOutOfFlowFrameForDisplay wouldn't descend, so maybe we should detect this case and always descend (as we will do when MarkPreserve3DFramesForDisplayList is called anyway).

This patch just checks if we will use a different dirty rect.

The other change isn't related to the try server failure, but I think its the correct thing to do.
Attachment #8767846 - Flags: review?(matt.woodrow)
Attachment #8767846 - Flags: review?(matt.woodrow) → review+
Duplicate of this bug: 1285123
Depends on: 1285409
Depends on: 1285411
The patches in bug 1285409 and bug 1285411 fix the preserve3d asserts without the hacky second patch here.
We're getting late in 48 beta, setting this as fix-optional. Are the patches you mention in comment 28 something you want to uplift or leave as they are to ride with 50?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #29)
> We're getting late in 48 beta, setting this as fix-optional. Are the patches
> you mention in comment 28 something you want to uplift or leave as they are
> to ride with 50?

Yes, these patches are upliftable and we should uplift them.
Flags: needinfo?(tnikkel)
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e09caf7c4e
Enforce that any items creating when we descend into an out-of-flow for which we don't have the proper clip are invisible. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/89e09caf7c4e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8763258 [details] [diff] [review]
hideinvisitems

Approval Request Comment
[Feature/regressing bug #]: bug 1265237
[User impact if declined]: extra cursor gunk in popular web based code editors
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: this should be pretty safe, if there are any regressions we should hear about them quickly
[String/UUID change made/needed]: none
Attachment #8763258 - Flags: approval-mozilla-beta?
Attachment #8763258 - Flags: approval-mozilla-aurora?
Comment on attachment 8763258 [details] [diff] [review]
hideinvisitems

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

This patch fixes a caret regression. Take it in 48 beta 8 and aurora.
Attachment #8763258 - Flags: approval-mozilla-beta?
Attachment #8763258 - Flags: approval-mozilla-beta+
Attachment #8763258 - Flags: approval-mozilla-aurora?
Attachment #8763258 - Flags: approval-mozilla-aurora+
This needs to be verified in beta 8.
Flags: qe-verify+
Duplicate of this bug: 1273346
I managed to reproduce this issue on Firefox 49.0a1 (2016-05-28) and on Windows 10 x64.
The issue is no longer reproducible on Firefox 48.0b9, Firefox 49.0a2 (2016-07-18) or on Firefox 50.0a1 (2016-07-18).
The tests were performed on Windows 10 x64, Ubuntu 16.04 x64, Mac OS X 10.12.
I am marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1289407
You need to log in before you can comment on or make changes to this bug.