Closed Bug 1274726 Opened 4 years ago Closed 3 years ago

Dragged tab's top edge has horizontal line rendering artifact during dragging when using a Firefox theme on OS X

Categories

(Core :: Graphics, defect)

48 Branch
Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed

People

(Reporter: cpeterson, Assigned: mchang)

References

Details

(Keywords: regression)

Attachments

(3 files)

[Tracking Requested - why for this release]:

Mason, I believe this bug is a regression from bug 1266933.

STR:
1. Install a light-colored Firefox theme, such as https://addons.mozilla.org/en-US/firefox/addon/soft-summer-evening/
2. Open a couple tabs.
3. Drag one of the tabs left and right

RESULT:
The dragged tab's top edge shifts down a pixel, making the tab look broken. Even before Skia bug 1266933 landed, a dragged tab's top edge looked blurred, but now it's much worse.

The bug doesn't seem to be reproducible with the default Firefox theme. I don't know whether that is because the default gray colors make seeing the problem difficult (even when zooming the screen) or installing any theme causes a problem. I can reproduce this problem with both light and dark themes, but the problem is easier to see with a light theme.
Flags: needinfo?(mchang)
Tracking this visual regression for 49. Chris - is this supposed to be nominated for tracking-firefox-esr38 as well?
Flags: needinfo?(cpeterson)
[Tracking Requested - why for this release]:

(In reply to Marcia Knous [:marcia - use ni] from comment #1)
> Tracking this visual regression for 49. Chris - is this supposed to be
> nominated for tracking-firefox-esr38 as well?

No. Thanks for catching that! I meant to nominate for tracking 48 and 49.
Flags: needinfo?(cpeterson)
Is it correct to say that this only happens when actually dragging the tab? When I lock the tab in place, the regression goes away.
Flags: needinfo?(mchang) → needinfo?(cpeterson)
(In reply to Mason Chang [:mchang] from comment #3)
> Is it correct to say that this only happens when actually dragging the tab?
> When I lock the tab in place, the regression goes away.

Yes. This problem only happens while dragging the tab.
Flags: needinfo?(cpeterson)
Summary: Dragged tab's top edge has horizontal line rendering artifact when using a Firefox theme on OS X → Dragged tab's top edge has horizontal line rendering artifact during dragging when using a Firefox theme on OS X
Assignee: nobody → mchang
Attached patch repeat.patchSplinter Review
When we're prescaling an image to repeat it, we first prescale the image then repeat the scaled image. During the prescaling transformation, we currently also extend the repeat mode [1]. Since we're only scaling the image, we can clamp the image during scaling, and only pass the repeat mode when we're actually repeating.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp?from=gfxUtils.cpp#606
Attachment #8755615 - Flags: review?(jmuizelaar)
Comment on attachment 8755615 [details] [diff] [review]
repeat.patch

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

Can you add a test for this?
Attachment #8755615 - Flags: review?(jmuizelaar) → review+
Attached patch ReftestSplinter Review
Specifically tests the prescale and repeat drawable on OS X only since that's the only place we do it. I'd be interested to see if we still have to do it anymore since Skia scales 3x faster than CG did.

Successful try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=097f514bc376
Attachment #8758940 - Flags: review?(jmuizelaar)
Attachment #8758940 - Flags: review?(jmuizelaar) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4b4972c8a5
Clamp scaled image before repeating during prescale and repeat on OS X. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe8a8bde7f0
Part 2: Reftest for prescale and repeat drawable. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/2b4b4972c8a5
https://hg.mozilla.org/mozilla-central/rev/0fe8a8bde7f0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8755615 [details] [diff] [review]
repeat.patch

Approval Request Comment
[Feature/regressing bug #]: Skia content on OS X
[User impact if declined]: Some graphics artifacts
[Describe test coverage new/current, TreeHerder]: Manual, attached reftest.
[Risks and why]: Low, we are forcing Skia to copy CoreGraphics behavior.
[String/UUID change made/needed]: None
Attachment #8755615 - Flags: approval-mozilla-beta?
Comment on attachment 8758940 [details] [diff] [review]
Reftest

See comment 12.
Attachment #8758940 - Flags: approval-mozilla-beta?
Comment on attachment 8755615 [details] [diff] [review]
repeat.patch

This fixes regression. Take it in beta.
Attachment #8755615 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8758940 [details] [diff] [review]
Reftest

This fixes regression. Take it in beta.
Attachment #8758940 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
has problems uplifting to beta:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 2b4b4972c8a5
grafting 348604:2b4b4972c8a5 "Bug 1274726. Clamp scaled image before repeating during prescale and repeat on OS X. r=jrmuizel"
merging gfx/thebes/gfxUtils.cpp
warning: conflicts while merging gfx/thebes/gfxUtils.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mchang)
Includes part 1 and part 2. I didn't have merge conflicts. From comment 16, it looks like you're applying it to mozilla-central?
Flags: needinfo?(mchang) → needinfo?(cbook)
(In reply to Mason Chang [:mchang] from comment #17)
> Created attachment 8763930 [details] [diff] [review]
> Rolled patch for beta
> 
> Includes part 1 and part 2. I didn't have merge conflicts. 
thanks this worked great !

From comment 16,
> it looks like you're applying it to mozilla-central?

oh not really, its a so called unified repo, that way we take whats landed on (in this case mozilla-central) and graft it to mozilla-beta, sorry for the confusion
Flags: needinfo?(cbook)
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.