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

Question: why layout values are rounded? #77

Closed
twop opened this issue May 31, 2022 · 5 comments
Closed

Question: why layout values are rounded? #77

twop opened this issue May 31, 2022 · 5 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@twop
Copy link

twop commented May 31, 2022

Just spent an hour debugging why the leaf node I measured is set to 63.0 when my measure function returned 63.375

When I looked at the sources I found the explicit rounding in the algo.rs

fn round_layout(
    nodes: &mut [NodeData],
    children: &[sys::ChildrenVec<NodeId>],
    root: NodeId,
    abs_x: f32,
    abs_y: f32,
) {
    let layout = &mut nodes[root].layout;
    let abs_x = abs_x + layout.location.x;
    let abs_y = abs_y + layout.location.y;

    layout.location.x = sys::round(layout.location.x);
    layout.location.y = sys::round(layout.location.y);

    layout.size.width = sys::round(abs_x + layout.size.width) - sys::round(abs_x);
    layout.size.height = sys::round(abs_y + layout.size.height) - sys::round(abs_y);

    for child in &children[root] {
        Self::round_layout(nodes, children, *child, abs_x, abs_y);
    }
}

I couldn't find any mentions why the rounding is needed, thus I'm curious. It seems to be 100% intentional. Maybe add an explanation in the code annotations?

@alice-i-cecile alice-i-cecile added the bug Something isn't working label May 31, 2022
@alice-i-cecile
Copy link
Collaborator

This is an inherited code base. Unless I can find directions in the spec to do this or behavior from Chrome that matches, I'm going to call this a bug.

@twop
Copy link
Author

twop commented May 31, 2022

One other observation:
From the MeasureFunc I returned (63.75, 10.5) but got back (63,10). Note the rounding of 10.5 down to 10, which feels a bit weird

Red box is the result of the layout, note that the text goes out of bounds because of the rounding.
image

@TimJentzsch
Copy link
Collaborator

I also can't find an explanation on why this was implemented in the related commits

@mockersf also edited parts of that code once in this commit, maybe they know more?

@mockersf
Copy link
Contributor

mockersf commented Jun 1, 2022

Browsers round fractional pixels. Some up, some down.

http://www.simonbattersby.com/blog/browsers-and-fractional-pixels/

This lib is made to work like Chrome so it's rounding like it.

@alice-i-cecile
Copy link
Collaborator

Browsers round fractional pixels. Some up, some down.

Ah, of course.

My instinct is that I'd like the ability to control whether or not we reproduce Chrome's bugs / quirks. But I'm not quite sure what design we should use for that. I'm not super keen on piping a config enum through everything, but a feature flag also seems very iffy.

Anyways, I'm going to close this out as "not a bug" for now, and we can open up a more focused issue for the "compatibility mode" question as it comes up.

@alice-i-cecile alice-i-cecile added the wontfix This will not be worked on label Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants