Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Replace Text Blur with PushTextShadow and PopTextShadow DisplayItems #1454

Merged
merged 2 commits into from Jul 12, 2017

Conversation

@Gankra
Copy link
Contributor

Gankra commented Jul 5, 2017

WIP because it doesn't implement the backend part of this -- someone else needs to do that. As before I've made a dummy variable that will create a warning so that the code compiles but points you to what code needs to be audited. I've also added some tests that capture some interesting cases (I suggest actually rendering them to make sure they look the way you want).

This is the new version of #1418, which instead of attaching shadows/decorations to text, uses a stateful PushTextShadow and PopTextShadow.

The semantics are that everything between a PushTextShadow and PopTextShadow should be rendered to a shadow with the given properties. (Should only expect glyphs, text-decorations, and blob-image versions of decorations here)

Why is this necessary?

Well, basically the CSS spec requires us to put every shadow in a given StackingContext on the same "layer", behind all the actual content. However we receive text at too low of a granularity to do this if the shadows are attached to the text. (we get roughly one TextItem per line, but more if fonts or styles change mid-line -- this can happen without any extra html/css elements due to font fallback on kanji or emoji)

The Push/Pop model gives us a simple way to say "hey all this content? yeah put a shadow on all of this". I expect the backend implementation model to look something like:

Before rendering any content in a given stacking context, for each PopTextShadow, render the enclosed elements with the given offset, blur, and color.

The PopTextShadow approach is also beneficial because it encourages us to put as much content as possible in the "same" shadow, preventing ugly artifacts from overlapping two parts of the "same" shadow. Technically the CSS spec gives us leeway to give each glyph its own shadow rendering, but we should try to avoid this as much as possible.

This approach also avoids us having to actually add a notion of text-decorations for the time-being. Although I think webrender should eventually have them, for now they can just be implemented as blob-images from firefox or boxes from servo. When we do add them, we also won't have to bloat text -- the client can just add separate top-level items for them.

r? @glennw (still up for the backend impl work?)


This change is Reviewable

@Gankra Gankra mentioned this pull request Jul 5, 2017
@glennw
Copy link
Member

glennw commented Jul 5, 2017

I'm up for the implementation work, but I've been thinking about this and wondering if a slightly different approach might be better.

If we go ahead and expose reference frames in the display list (#1443) which I think makes sense, then having the concept of a stacking context directly in the display list is basically a superset of the PushTextShadow item, I think.

Effectively, I'm suggesting that instead of PushStackingContext and PushTextShadow, we replace these by a "PushLayerGroup" (for lack of a better name right now) that specifies a list of CSS filters / mix-blend-modes / shadows, and that this one display item is then used by the DL builder for both text shadows and stacking contexts that have filters / blends on them.

Thoughts @kvark @mrobinson @gankro @jrmuizel ?

@glennw
Copy link
Member

glennw commented Jul 5, 2017

Although, just thinking that through a bit more, I guess we still need the concept of a stacking context specifically, in order to pass things like preserve-3d. Hmmm, I will think about this a bit more today.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 5, 2017

I have no particular preference for if this is a dedicated thing or part of a broader notion of "groups" -- other than a vague desire to avoid incredibly bloated "swiss-army-knife" display items with a dozen optional properties

@kvark
Copy link
Member

kvark commented Jul 6, 2017

@glennw yep, sounds like Push/PopStackingContext is here to stay. I think Push/PopTextShadow is fine as long as there are no stacking contexts inside those shadow sections.

@glennw
Copy link
Member

glennw commented Jul 9, 2017

I'm working on some other text-related issues. Once those are fixed, I'll start work on the implementation side of this (hopefully this week).

@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2017

The latest upstream changes (presumably #1428) made this pull request unmergeable. Please resolve the merge conflicts.

@Gankra Gankra force-pushed the Gankra:push-shadow branch from f9d7b43 to 089818f Jul 10, 2017
Gankra added a commit to Gankra/servo that referenced this pull request Jul 11, 2017
Also includes an incidental bugfix for the rendering order of text decorations.

This change depends on servo/webrender#1454 being merged and upstreamed to servo.
@glennw
Copy link
Member

glennw commented Jul 12, 2017

PR with the implementation (and the commits from here included) is #1469.

@bors-servo bors-servo merged commit 089818f into servo:master Jul 12, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Gankra added a commit to Gankra/servo that referenced this pull request Jul 13, 2017
Also includes an incidental bugfix for the rendering order of text decorations.

This change depends on servo/webrender#1454 being merged and upstreamed to servo.
glennw pushed a commit to glennw/servo that referenced this pull request Jul 17, 2017
Also includes an incidental bugfix for the rendering order of text decorations.

This change depends on servo/webrender#1454 being merged and upstreamed to servo.
glennw pushed a commit to glennw/servo that referenced this pull request Jul 18, 2017
Also includes an incidental bugfix for the rendering order of text decorations.

This change depends on servo/webrender#1454 being merged and upstreamed to servo.
mrobinson added a commit to mrobinson/servo that referenced this pull request Jul 18, 2017
Also includes an incidental bugfix for the rendering order of text decorations.

This change depends on servo/webrender#1454 being merged and upstreamed to servo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.