811825 - Reflow-on-zoom should not constrain the max line box width for constrained height elements

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Readability, defect)

Description

[Scott Johnson (:jwir3)]

Right now, we go ahead and make the max line box width smaller for a frame that has constrained height. This means that it will clip the frame vertically if it exceeds this height after rewrapping text. 

Instead, we should disable the reflowing (i.e. the constraining of line box width) if the constrained height bit is set for the frame.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b811825

Comment 2

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

Comment on attachment 682163 [details] [diff] [review]
b811825

>+  // If we're in a constrained height frame, then we don't allow a
>+  // max line box width to take effect.
>+  if (!(aFrame->GetContainingBlock()->GetStateBits() & NS_FRAME_IN_CONSTRAINED_HEIGHT)) {

Instead of aFrame->GetContainingBlock(), use GetLineContainerFrame() (which is a method on |this|).

Also, wrap after the &, since you're going over 80 chars.

(I'm also curious why this code is in ReflowFrame rather than in BeginLineReflow, since I'd think we only need to do this once per line.)

>+    if (maxLineBoxWidth > 0
>+      && psd->mRightEdge - psd->mLeftEdge > maxLineBoxWidth) {

Put && at end of line rather than start, and then line up "psd" with "max".

Comments on the test to follow.

Comment 3

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

Comment on attachment 682163 [details] [diff] [review]
b811825

So I'm not crazy about the test here; it's starting to look pretty fragile.  This seems like something that would perhaps better be done with the reftest harness, and putting these separate tests in actually separate test files.  Or perhaps something else to keep the tests from interacting with each other in weird ways -- it's the setting display:none on the parts of the test you're not currently using that I'd like to see go away.

r=dbaron overall, though (see previous comment too), but it would be good to improve the test

Comment 4

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #3)
> Comment on attachment 682163 [details] [diff] [review]
> b811825
> 
> So I'm not crazy about the test here; it's starting to look pretty fragile. 

Agreed. I can see if I can refactor it a bit and perhaps make it more stable.

> This seems like something that would perhaps better be done with the reftest
> harness, and putting these separate tests in actually separate test files. 

I'd much rather do this, but I was under the impression that since the JS needed chrome privileges, that the only way to do it was to put it in mochitest-chrome.  Come to think of it, though, could I do this with the special permissions API in the reftest harness? 

> Or perhaps something else to keep the tests from interacting with each other
> in weird ways -- it's the setting display:none on the parts of the test
> you're not currently using that I'd like to see go away.

Well, one thing I could do is just put all of the tests that use separate content into separate mochitests. That would (hopefully) eliminate some of the potential for interaction.

> r=dbaron overall, though (see previous comment too), but it would be good to
> improve the test

I'll see what I can do about improving the test. I will also address the comments made in comment 2.

Comment 5

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

(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to David Baron [:dbaron] from comment #3)
> > Comment on attachment 682163 [details] [diff] [review]
> > b811825
> > 
> > So I'm not crazy about the test here; it's starting to look pretty fragile. 
> 
> Agreed. I can see if I can refactor it a bit and perhaps make it more stable.
> 
> > This seems like something that would perhaps better be done with the reftest
> > harness, and putting these separate tests in actually separate test files. 
> 
> I'd much rather do this, but I was under the impression that since the JS
> needed chrome privileges, that the only way to do it was to put it in
> mochitest-chrome.  Come to think of it, though, could I do this with the
> special permissions API in the reftest harness? 

Oh, that's a problem.

> > Or perhaps something else to keep the tests from interacting with each other
> > in weird ways -- it's the setting display:none on the parts of the test
> > you're not currently using that I'd like to see go away.
> 
> Well, one thing I could do is just put all of the tests that use separate
> content into separate mochitests. That would (hopefully) eliminate some of
> the potential for interaction.

The other thing is to have the reftests in separate files, but still have the mochitest drive them.  Probably the best example of this is the way we used to run the font-inflation reftests prior to bug 743817.

Comment 6

[Scott Johnson (:jwir3)]

So, I think what I'm going to do is open another bug for the test changes. David, as you suggest, I'll make a mochitest harness much like you did for font-inflation initially. 

As for the code changes (Reflow vs. BeginLineReflow), I'll make those as you recommend in this patch. 

Do you want to re-review the changes after I've made them?

Comment 7

[Scott Johnson (:jwir3)]

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

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fff69f25ad6c