Should multiply border width by the stacking context's scale

RESOLVED FIXED in mozilla57

Status

()

Core
Graphics: WebRender
P1
normal
RESOLVED FIXED
a month ago
27 days ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected)

Details

(Whiteboard: [wr-mvp])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a month ago
We store the scale value to stacking context. But currently we only use it to calculate the bounds and clipping. Border width is another thing we should take care. A related test is "layout/reftests/bugs/586683-1.html".
(Assignee)

Comment 1

a month ago
I think only border width has this problem.
Summary: Border width should multiply the stacking context's scale → Should multiply border width by the stacking context's scale
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8906956 - Flags: review?(bugmail)
(Assignee)

Updated

a month ago
Attachment #8906957 - Flags: review?(bugmail)
(Assignee)

Comment 4

a month ago
Sorry, I didn't think it clearly. Per discussion with Markus, border radius, box shadow, and text may also need to be scaled.

Updated

a month ago
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a month ago
I haven't figured out a very good way to apply the scale to all items. My current approach is to let stacking context have the ability to scale different wr properties. The current patches are for border renderer. If this way is acceptable, then I can use the same way to apply other wr commands in the following patches or bugs.
I'm not really convinced that this is the right approach to be taking. If we go down this road and scale everything on the gecko side things are going to get very complicated. It's still not clear to me why the scaling doesn't happen inside WebRender. I think we should investigate that in more detail and see if there's something we can change in WebRender to make this work. If not, we should at least properly document the problem so that it's easier to understand what things need to be scaled on the Gecko side.
(Assignee)

Comment 9

a month ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> I'm not really convinced that this is the right approach to be taking. If we
> go down this road and scale everything on the gecko side things are going to
> get very complicated. It's still not clear to me why the scaling doesn't
> happen inside WebRender. I think we should investigate that in more detail
> and see if there's something we can change in WebRender to make this work.
> If not, we should at least properly document the problem so that it's easier
> to understand what things need to be scaled on the Gecko side.

Originally we set the scale to the stacking context's transform. So everything of the item will be scaled. In bug 1391499, we retrieve the scale from the transform for reordering the transform, and then manually apply the scale to the bounds (StackingContextHelper::ToRelativeLayoutRectRounded). So the problem is we should apply the scale to the whole item, not just bounds. An alternative way is that the reordering things probably can be done in Webrender. Then we don't need to handle the inherited scale on the gecko site.
Yeah I'm starting to think what we did in bug 1391499 was not the correct approach. I'll spend some more time investigating this as well. I'm going to back out those patches locally and see if we can make it work a different way.
(Assignee)

Updated

a month ago
Attachment #8906956 - Flags: review?(bugmail)
(Assignee)

Updated

a month ago
Attachment #8906957 - Flags: review?(bugmail)
Depends on: 1400034
status-firefox57: --- → unaffected
Comment hidden (mozreview-request)
(Assignee)

Updated

28 days ago
Attachment #8906957 - Attachment is obsolete: true
(Assignee)

Comment 12

28 days ago
After bug 1400034 landed, the test is passed. So I just turn on layers-free for the test in this bug.

Comment 13

28 days ago
mozreview-review
Comment on attachment 8906956 [details]
Bug 1399043 - Turn on layers-free mode for the test about border with scale.

https://reviewboard.mozilla.org/r/178700/#review187062
Attachment #8906956 - Flags: review?(bugmail) → review+
(Assignee)

Comment 14

27 days ago
"layers-free" is going to default on. I think I don't need to land this patch anymore.
Status: ASSIGNED → RESOLVED
Last Resolved: 27 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.