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

RESOLVED FIXED in Firefox 48

Status

()

Core
Graphics
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: cpeterson, Assigned: mchang)

Tracking

({regression})

48 Branch
mozilla49
Unspecified
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(firefox46 unaffected, firefox47 unaffected, firefox48+ fixed, firefox49+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

a year ago
[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?
tracking-firefox49: ? → +
Flags: needinfo?(cpeterson)
(Reporter)

Comment 2

a year ago
[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.
tracking-firefox48: --- → ?
tracking-firefox-esr38: ? → ---
Flags: needinfo?(cpeterson)
(Assignee)

Comment 3

a year ago
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)
(Reporter)

Comment 4

a year ago
(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
Tracking for 48.
tracking-firefox48: ? → +
(Assignee)

Updated

a year ago
Assignee: nobody → mchang
(Assignee)

Comment 6

a year ago
Created attachment 8755615 [details] [diff] [review]
repeat.patch

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)
(Assignee)

Comment 7

a year ago
Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b1bbe4c32c4
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+
(Assignee)

Comment 9

a year ago
Created attachment 8758940 [details] [diff] [review]
Reftest

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+

Comment 10

a year ago
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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b4b4972c8a5
https://hg.mozilla.org/mozilla-central/rev/0fe8a8bde7f0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 12

a year ago
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?
(Assignee)

Comment 13

a year ago
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)
(Assignee)

Comment 17

a year ago
Created attachment 8763930 [details] [diff] [review]
Rolled patch for beta

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)

Comment 18

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9e4f55640864
status-firefox48: affected → fixed
(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.