803211 - Max line box width API should not limit line boxes based on absolute position

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

Right now, the following code exists in the API for changing the max line box width:

>   if (maxLineBoxWidth > 0 && psd->mRightEdge > maxLineBoxWidth) {
>    psd->mRightEdge = maxLineBoxWidth;
>  }

This effectively limits a max line boxes' width based on an absolute position, which is causing reflow-on-zoom to be overly conservative when limiting line box widths.  Instead, it should take into account the WIDTH of the line box, not the position of the right edge.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b803211

Comment 2

[Scott Johnson (:jwir3)]

Comment on attachment 672899 [details]
b803211

Going to add a test, so will repost shortly.

Comment 3

[Scott Johnson (:jwir3)]

Attachment: b803211

Now with a mochitest!

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 673415 [details] [diff] [review]
b803211

r=dbaron, though I think the test is a bit confusing with part of it in functions and part of it not in functions (especially given the order in which the functions and the other tests live within the file).  Maybe it can be organized a bit more clearly?

Oh, and I now realize RTL is even worse than I thought:
 * it's not clear to me that positioning things at the left edge is the right thing for RTL
 * RTL + text-indent is also different from what I originally thought; given that this code adjusts mRightEdge just like text-indent, we end up erasing the text indent (rather than indenting out the other end, as I was originally worried)

I think you should probably get a bug filed for improving RTL behavior here.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

[oops, I thought I'd submitted this comment before the last one!]

(In reply to Scott Johnson (:jwir3) from comment #0)
> This effectively limits a max line boxes' width based on an absolute
> position, which is causing reflow-on-zoom to be overly conservative when

This had me confused for a bit, because I thought you were using "absolute position" in the sene of CSS absolute positioning.  But now I realize that I think you just meant it as the English term.

So this change looks reasonable, except after poking around nsLineLayout a bit to look into what mLeftEdge and mRightEdge are, I realize that this is still broken in the presence of mTextIndent when direction is RTL (since we handle text-indent asymmetrically).

Comment 6

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #4)
> Comment on attachment 673415 [details] [diff] [review]
> b803211
> 
> r=dbaron, though I think the test is a bit confusing with part of it in
> functions and part of it not in functions (especially given the order in
> which the functions and the other tests live within the file).  Maybe it can
> be organized a bit more clearly?

I re-organized the test so that everything is in functions now. There are four functions: setupFirstTest(), setupSecondTest(), performFirstTest(), and performSecondTest().

> I think you should probably get a bug filed for improving RTL behavior here.

Filed bug 804640 for this.

Comment 7

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1b86ceab1a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Note that this landed with the wrong bug number in the commit message...
https://hg.mozilla.org/mozilla-central/rev/fd1b86ceab1a