Skip to content

/ servo

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

Implement root element backgrounds #19704

Closed
wants to merge 2 commits into from
Closed

Implement root element backgrounds #19704

wants to merge 2 commits into from

Conversation

@pyfisch
Copy link
Contributor

pyfisch commented Jan 5, 2018 •
edited by SimonSapin

Note: #19669 is visible with background images (and some gradients).

Note 2: Maybe the root background only needs to be propagated on REPAINT?

Note 3: I am not sure what the complete implications of not sending a background color to webrender are. But setting one gives wrong results with partially transparent background colors.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #19421
  • There are tests for these changes

This change is Reviewable
@emilio
emilio reviewed Jan 6, 2018
View changes
components/layout_thread/lib.rs Outdated
return;
}
let root_block = root_flow.as_mut_block();
let root_style = ServoArc::make_mut(&mut root_block.fragment.style);

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Jan 6, 2018

Member

It looks unfortunate to mutate the style from here... We should at least move the make_mut to where we know we are going to change it.

This comment has been minimized.

Sign in to view
Copy link
@pyfisch

pyfisch Jan 6, 2018

Author Contributor

It looks unfortunate to mutate the style from here

Where should it be done instead?
components/layout_thread/lib.rs Outdated
.resolve_color(kid_block_flow.fragment.style.get_background().background_color)
.to_gfx_color()
let kid_mut_block = kid.as_mut_block();
let kid_background = ServoArc::make_mut(&mut kid_mut_block.fragment.style).mutate_background();

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Jan 6, 2018

Member

I'm pretty sure this make_mut is not needed, is it?

Why wouldn't just kid_mut_block.fragment.style.get_background() suffice?

But more importantly:

  • What guarantees that the <body> is a block?
  • What guarantees that it's the first block?

Not that the code before wasn't wrong, but...

This comment has been minimized.

Sign in to view
Copy link
@pyfisch

pyfisch Jan 6, 2018

Author Contributor

I'm pretty sure this make_mut is not needed, is it?
Why wouldn't just kid_mut_block.fragment.style.get_background() suffice?

Well the borrow checker disagrees. In lines 1709 and 1710 kid_background is mutated. This prompts the compiler to tell me cannot borrow immutable borrowed content `*kid_background` as mutable.

What guarantees that the is a block?

Good catch. While the <body> is usually a block with body { display: inline; } it is not. Which other FlowClass besides block and inline can it be?

What guarantees that it's the first block?

The body element is always the first block. (Even if there is no <body> tag in the source code. Or the code is something like <a href="https://mozilla.org">Mozilla</a><body>Main content</body>.)
There must be some specifications that describes this. One part is Optional Tags. Another is HTML parsing The "after head" insertion mode and HTML parsing The "in body" insertion mode. Servo seems to follow all these weird rules as far as I can tell.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Jan 6, 2018

Member

The body element is always the first block. (Even if there is no tag in the source code. Or the code is something like MozillaMain content.)

I'm pretty sure I can do head { display: block }.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Jan 6, 2018

Member

Also, with script I can do all sorts of stuff.

This comment has been minimized.

Sign in to view
Copy link
@pyfisch

pyfisch Jan 6, 2018

Author Contributor

I'm pretty sure I can do head { display: block }.

Yes this breaks my assumption "always first block". And afaik I can't even ask flows if they are a body element.
@pyfisch
Implement root element backgrounds
Loading status checks…
c5a5162 
Closes  #19421
@pyfisch pyfisch force-pushed the pyfisch:root-background branch from b7ad4d0 to c5a5162 Jan 6, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 6, 2018

I've pushed an updated version. It works with head { display: block }. But as you say it is possible to do all sorts of stuff with script, so it may fail here.
@emilio
emilio reviewed Jan 6, 2018
View changes
Copy link
Member

emilio left a comment

I think we should fix this properly. Fragments contain the OpaqueNode. We should be able to get the document body from TDocument in layout_thread, and stash it as an OpaqueNode in LayoutContext.

I also think that just mutating the fragment's style using make_mut is not great...
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 7, 2018 •
edited

Fragments contain the OpaqueNode. We should be able to get the document body from TDocument in layout_thread.

I have done this. For block-like elements I can now check if block.fragment.node equals the OpaqueNode from body. But an InlineFlow may have multiple associated fragments. What is correct here?

BTW the correct way to get a document body is described at https://html.spec.whatwg.org/multipage/dom.html#dom-document-body.
@emilio
Copy link
Member

emilio commented Jan 7, 2018

I think for inlines you need to iterate all the fragments and find the one corresponding to the body, if any. @mbrubeck can correct me here if I'm wrong.
@pyfisch
WIP Changes to determine_root_background
Loading status checks…
89be5f5
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 7, 2018

Unfortunately the InlineFragments have a different OpaqueId from the body. Besides if the body is an inline and empty there is not even an InlineFlow created. If the body has multiple lines each one has an individual background gradient.

I feel that is the wrong place to implement this. Probably it should be done in style or wherever the ComputedValues are assigned to elements.

For testing:

<!doctype html>
<style>
    body {
        display: inline;
        background-image: linear-gradient(to right, white, black);
    }
</style>
<body>
    Hello World!<br>
    And a Happy New Year
</body>
@pyfisch pyfisch closed this Jan 7, 2018
@pyfisch pyfisch mentioned this pull request Jan 7, 2018
Open
@pyfisch pyfisch mentioned this pull request Mar 25, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@asajeffrey asajeffrey

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

No background image on http://bholley.net/
4 participants
@pyfisch @emilio @asajeffrey @highfive
You can’t perform that action at this time.