Last Comment Bug 898797 - Use ApplyRelativePositioning when placing floats
: Use ApplyRelativePositioning when placing floats
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Corey Ford [:coyotebush]
:
Mentors:
Depends on: 893962
Blocks: 886646 898794
  Show dependency treegraph
 
Reported: 2013-07-27 18:45 PDT by Corey Ford [:coyotebush]
Modified: 2013-08-01 13:57 PDT (History)
1 user (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reftests for relatively positioned floats. (5.16 KB, patch)
2013-07-29 17:56 PDT, Corey Ford [:coyotebush]
dholbert: review+
Details | Diff | Review
Reftests for relatively positioned floats. (v2) (6.07 KB, patch)
2013-07-29 18:35 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Review
Use nsHTMLReflowState::ApplyRelativePositioning when placing floats. r= (6.47 KB, patch)
2013-07-29 22:04 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Review

Description Corey Ford [:coyotebush] 2013-07-27 18:45:08 PDT
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.
Comment 1 Corey Ford [:coyotebush] 2013-07-29 14:57:00 PDT
This turns out to be the only use of ComputedOffsetProperty/GetRelativeOffset that I'm not sure how I can get rid of.
Comment 2 Corey Ford [:coyotebush] 2013-07-29 17:56:47 PDT
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.
Comment 3 Daniel Holbert [:dholbert] 2013-07-29 18:30:00 PDT
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!
Comment 4 Corey Ford [:coyotebush] 2013-07-29 18:35:49 PDT
Created attachment 782900 [details] [diff] [review]
Reftests for relatively positioned floats. (v2)

Addressed those comments.
Comment 5 Corey Ford [:coyotebush] 2013-07-29 22:04:07 PDT
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.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-30 23:23:08 PDT
Comment on attachment 782952 [details] [diff] [review]
Use nsHTMLReflowState::ApplyRelativePositioning when placing floats. r=

r=dbaron

Note You need to log in before you can comment on or make changes to this bug.