The default bug view has changed. See this FAQ.

Use ApplyRelativePositioning when placing floats

RESOLVED FIXED in mozilla25

Status

()

Core
Layout: R & A Pos
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: coyotebush, Assigned: coyotebush)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
https://hg.mozilla.org/mozilla-central/file/75e2498668df/layout/generic/nsBlockReflowState.cpp#l820 is one case I arguably missed in bug 893962.

This should use nsHTMLReflowState::ApplyRelativePositioning, though I'm not actually sure where to find the computed offsets if not in the frame property (which bug 898794 would otherwise eliminate). It doesn't look like mReflowState would contain the right ones at this point in the code.

It's also worth considering what we want the behavior of sticky positioned floats to be. I'd guess it should be identical to position:relative (so it sticks, but doesn't re-wrap while scrolling), and I assume that's what I would get by making this change. For what it's worth, though, WebKit appears to ignore the position:sticky declaration on floats.
(Assignee)

Comment 1

4 years ago
This turns out to be the only use of ComputedOffsetProperty/GetRelativeOffset that I'm not sure how I can get rid of.
Blocks: 898794
(Assignee)

Comment 2

4 years ago
Created attachment 782886 [details] [diff] [review]
Reftests for relatively positioned floats.

Before I touch that line, how about a couple reftests to at least make sure relative positioning of floats works (nothing fancy)?

These break if I comment out that line.
Attachment #782886 - Flags: review?(dholbert)
Comment on attachment 782886 [details] [diff] [review]
Reftests for relatively positioned floats.

>+++ b/layout/reftests/floats/relative-float-1-noref.html
>@@ -0,0 +1,2 @@
>+<!DOCTYPE html>
>+<div style="float: left; position: relative; height: 100px; width: 100px; background: blue"></div>

I'm not sure how much this "-noref" file adds; the "float:left; position:relative" don't actually have any effect, so it's basically a test that the reference case's relpos div doesn't match this non-positioned div. And I'd wager we have tests that cover that already, so I don't think it's necessary to test it here.

If you'd strongly like to, I won't object; but in general, I tend to avoid "-noref" files because:
 (a) it's hard to predict what sorts of "norefs" would actually be useful for catching bugs.
 (b) the things tested in a -noref are generally better tested directly (i.e. in a "does relative positioning of a div work at all?" test), and they usually already are tested elsewhere.
 (c) Given (a) and (b), I don't think the benefit outweighs the cost of running the test on every supported platform manymany times per day from now until forever.

>diff --git a/layout/reftests/floats/relative-float-1-ref.html b/layout/reftests/floats/relative-float-1-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/floats/relative-float-1-ref.html
>@@ -0,0 +1,2 @@
>+<!DOCTYPE html>
>+<div style="position: relative; top: 20px; left: 20px; height: 100px; width: 100px; background: blue"></div>

This applies to all of the files in this patch:
 - Per IRC discussion, I'd prefer that chunk of style in a <style> block, with each declaration on its own line, so that the testcase and reference case are easier to compare with 'diff' or a merge tool. (to see what we're testing)
 - Please add the public domain header from the bottom of https://www.mozilla.org/MPL/headers/  (not the MPL header) to the testcases & reference cases, so that other folks can repurpose these tests if they like.

r=me with that. Thanks for finding this relatively-untested codepath and adding tests for it!
Attachment #782886 - Flags: review?(dholbert) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 782900 [details] [diff] [review]
Reftests for relatively positioned floats. (v2)

Addressed those comments.
(Assignee)

Updated

4 years ago
Attachment #782886 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #782900 - Flags: review+
(Assignee)

Comment 5

4 years ago
Created attachment 782952 [details] [diff] [review]
Use nsHTMLReflowState::ApplyRelativePositioning when placing floats. r=

Ah, I can pull the offsets out of nsBlockFrame::ReflowFloat. This passes my new tests just fine.
Attachment #782952 - Flags: review?(dbaron)
Comment on attachment 782952 [details] [diff] [review]
Use nsHTMLReflowState::ApplyRelativePositioning when placing floats. r=

r=dbaron
Attachment #782952 - Flags: review?(dbaron) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/07d92b28f721
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b6963f6cec
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07d92b28f721
https://hg.mozilla.org/mozilla-central/rev/b3b6963f6cec
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.