Closed Bug 1105137 Opened 9 years ago Closed 9 years ago

The text appears behind the image

Categories

(Core :: Layout, defect)

36 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: over68, Assigned: smontagu)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

This problem occurs since the version 36.0a1 (2014-10-23).

https://hg.mozilla.org/mozilla-central/rev/88adcf8fef83
[Tracking Requested - why for this release]:

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/506226a2e6c0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141021202933
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d7da5ff1ed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141021211530
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=506226a2e6c0&tochange=f1d7da5ff1ed

Regressed by: Bug 1062963
Blocks: 1062963
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
Keywords: regression
(In reply to blinky from comment #0)
> The text appears behind the image in page
> https://dl.dropboxusercontent.com/u/95157096/85f61cf7/ItJnkL8E9x.html.

This link simply redirects to a live website that looks likely to change at any time. A persistent (and preferably reduced!) testcase would be great.
Keywords: testcase-wanted
I think this bug occurs with the "direction: rtl", See https://dl.dropboxusercontent.com/u/95157096/85f61cf7/1DVYf03H9E.png.

Without the "direction: rtl" https://dl.dropboxusercontent.com/u/95157096/85f61cf7/wjrN2jXaQQ.png.
Attached file a testcase
Text overlaps a image

same regression range.
Simon, can you help here? thanks
Flags: needinfo?(smontagu)
Assignee: nobody → smontagu
Flags: needinfo?(smontagu)
This is more or less the minimal fix for the bug, but I wonder if it would make sense to add another logical class, LogicalDimension or some such, which just contains an inline and a block nscoord, and doesn't use a container width for any conversions. WDYT?

The overloading of LogicalPoint for cases where it's an offset, not a point in any coordinate space, has confused me in the past -- see bug 1089581 comment 2.
Attachment #8534402 - Flags: review?(jfkthame)
Attached patch Reftest based on the testcase (obsolete) — Splinter Review
Attachment #8534404 - Flags: review?(jfkthame)
Comment on attachment 8534404 [details] [diff] [review]
Reftest based on the testcase

Review of attachment 8534404 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/bugs/1105137-1.html
@@ +15,5 @@
> +  <body>
> +    <div dir="ltr">
> +      <p>bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla bla </p>
> +      <div class="float">
> +        <img src="https://bugzilla.mozilla.org/skins/standard/index/help.png" width="240" height="180">

This looks cool when viewing the test, but for landing in the tree we shouldn't refer to an external resource.

Either use a local image, or omit it altogether: if you just say

  <img src="foo" width="240" height="180">

the test will still work, it'll just display a broken-image icon in the frame.
Comment on attachment 8534402 [details] [diff] [review]
Treat nsFloatManager's mOrigin as an offset, not a point, and rename it to mOffset to make that clearer

Review of attachment 8534402 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

If you'd like to add a followup patch to introduce a new type, and switch mOffset over to use it, I'd be fine with that too. I don't much like LogicalDimension as a name, though; I'd suggest LogicalOffset or LogicalVector as possibilities - wdyt?
Attachment #8534402 - Flags: review?(jfkthame) → review+
Attached patch Reftest v.2Splinter Review
Changed to use <img src="foo" ...>

Amusingly enough, that forced me to make another change and force the direction of the image, because the broken-image icon is direction-dependent ;-)
Attachment #8534404 - Attachment is obsolete: true
Attachment #8534404 - Flags: review?(jfkthame)
Attachment #8534462 - Flags: review?(jfkthame)
Attachment #8534462 - Flags: review?(jfkthame) → review+
Comment on attachment 8534402 [details] [diff] [review]
Treat nsFloatManager's mOrigin as an offset, not a point, and rename it to mOffset to make that clearer

Approval Request Comment
[Feature/regressing bug #]: Bug 1062963
[User impact if declined]: Visible regressions on right-to-left pages with text concealed behind floats
[Describe test coverage new/current, TBPL]: Baked on trunk since 2014-12-11, reftest included in checkin
[Risks and why]: low-risk, only change is to the coordinates of layout objects.
[String/UUID change made/needed]: N/A
Attachment #8534402 - Flags: approval-mozilla-aurora?
Hmmm, I think the original testcase from comment 0 still shows a bug, although attachment 8529100 [details] and the testcase from comment 4 do seem to be fixed.
Flags: needinfo?(alice0775)
Attachment #8534402 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1113526
(In reply to Simon Montagu :smontagu from comment #15)
> Hmmm, I think the original testcase from comment 0 still shows a bug,
> although attachment 8529100 [details] and the testcase from comment 4 do
> seem to be fixed.

Filed Bug 1113526
Flags: needinfo?(alice0775)
Blocks: 1142318
This was backed out from Fx37 as part of addressing bug 1114329.
You need to log in before you can comment on or make changes to this bug.