706609 - drop downs do not resize to the text properly when in portrait mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: screenshot

1. install latest nightly  (20111130)
2. set device to portrait mode
3. go to http://people.mozilla.com/~nhirata/html_tp/formsninput.html
4. look at dropdown

Expected: to be able to read the current selection
Actual: the dropdown is not resized to the text.

Note:
1. does not occur on the default browser

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: default web browser screenshot

Comment 2

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

Yeah, this requires changes to nsListControlFrame.

Comment 4

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

So I think the best way to proceed here, since a combobox contains a listbox in its dropdown, is to:
 1. fix listboxes
 2. fix comboboxes, if the fix for (1) doesn't already fix comboboxes

I tried https://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/fdf7f5b496e4/font-inflation-selects but it doesn't work yet, though; I haven't yet had a chance to debug why.

Comment 5

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

Handling nsSelectsAreaFrame in IsContainerForFontSizeInflation:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/be2789ce52ad/font-inflation-selects
fixes the text size and height issues for both list boxes and comboboxes, but widths are still wrong.  That probably does require hacking list controls and or comboboxes.

Comment 6

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

I think the width calculation for dropdowns is just nsContainerFrame::ComputeAutoSize and nsFrame::ShrinkWidthToFit.  Ideally, ComputeAutoSize would consider inflation throughout when we're in subtrees that don't contain inflation containers, but it depends on GetMinWidth and GetPrefWidth which, in general, can't.

Comment 7

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

(In reply to David Baron [:dbaron] from comment #6)
> I think the width calculation for dropdowns is just
> nsContainerFrame::ComputeAutoSize and nsFrame::ShrinkWidthToFit.  Ideally,
> ComputeAutoSize would consider inflation throughout when we're in subtrees
> that don't contain inflation containers, but it depends on GetMinWidth and
> GetPrefWidth which, in general, can't.

Er, really, it's not specific to ComputeAutoSize:  it's just about doing
intrinsic width calculations in subtrees that don't contain inflation
containers.

Comment 8

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

The simplest solution to that problem I can think of is:
 * add a frame state bit for whether a frame is a container for font size inflation, and set it during frame construction so we have it cheaply in the future
 * add a boolean parameter to GetPrefWidth, GetMinWidth, and AddInline{Min,Pref}Width that says whether to consider font size inflation.  Set it to false at the very start of the method for anything that's an inflation container; then pass it down to descendants.

Comment 9

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

Attachment: Add a frame state bit for whether a frame is a container for font size inflation (, patch 1)

Comment 10

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

Attachment: Add member variables to pres context to track, during reflow and intrinsic sizing, the current inflation container and its width. (, patch 2)

Comment 11

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

Attachment: Set inflation container and width correctly during reflow. (, patch 3)

Comment 12

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

Attachment: Set inflation container to null during parts of intrinsic sizing that should not have inflation applied. (, patch 4)

Comment 13

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

Attachment: Switch nsLayoutUtils inflation methods to the new setup with state on the pres context. (, patch 5)

Comment 14

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

Attachment: Honor inflation during intrinsic width computation when needed. (, patch 6)

Comment 15

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

Attachment: Apply font size inflation to 'line-height: normal' by using the metrics of the inflated font rather than multiplying the inflation by the result for the uninflated font. This matters primarily because it makes font inflation easier to test by avoiding ro

Comment 16

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

Attachment: Make selects and things inside them not be containers for font size inflation, so that font inflation inflate selects appropriately along with what surrounds them. (, patch 8)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 589941 [details] [diff] [review]
Add a frame state bit for whether a frame is a container for font size inflation (, patch 1)

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

::: layout/generic/nsFrame.cpp
@@ +519,5 @@
> +#endif
> +      ) {
> +    if (IsFontSizeInflationContainer(this)) {
> +      mState |= NS_FRAME_FONT_INFLATION_CONTAINER;
> +    }

Pass nsStyleDisplay as a parameter to IsFontSizeInflationContainer and reuse the call to GetStyleDisplay() for transforms.

Comment 18

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

Attachment: Add a frame state bit for whether a frame is a container for font size inflation (, patch 1)

In addition to addressing comment 17, this also adds an exception to an assertion for br frames, since the assertion was firing in a harmless way.

Comment 19

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

Attachment: Make floating :first-letter frames not be containers for font size inflation, to avoid triggering assertions. (, patch 3.5)

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 589944 [details] [diff] [review]
Set inflation container to null during parts of intrinsic sizing that should not have inflation applied.  (, patch 4)

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

::: layout/base/nsLayoutUtils.cpp
@@ +2240,5 @@
> +  if (nsLayoutUtils::IsContainerForFontSizeInflation(aFrame)) {
> +    // If aFrame is a container for font size inflation, then shrink
> +    // wrapping inside of it should not apply font size inflation.
> +    presContext->mCurrentInflationContainer = nsnull;
> +  }

What about having a slightly different and more efficient AutoRestore API that doesn't save and restore unless the condition is true? e.g.
  AutoRestore<T> ar;
  if (cond) {
    ar.SaveAndSet(&value, v);
  }

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Better still, have dedicated AutoSetInflationContainer(nsIFrame*) class that encapsulates this into one line.

Comment 22

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> What about having a slightly different and more efficient AutoRestore API
> that doesn't save and restore unless the condition is true? e.g.
>   AutoRestore<T> ar;
>   if (cond) {
>     ar.SaveAndSet(&value, v);
>   }

That's more complicated, since then the restoring has to be conditional too and the condition has to be stored.  Feels like over-optimizing to me.

Comment 23

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Better still, have dedicated AutoSetInflationContainer(nsIFrame*) class that
> encapsulates this into one line.

I suppose that's probably worth it, though.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 589945 [details] [diff] [review]
Switch nsLayoutUtils inflation methods to the new setup with state on the pres context.  (, patch 5)

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

::: layout/base/nsLayoutUtils.h
@@ +1490,5 @@
>     * smaller than 1.
>     *
> +   * The WidthDetermination parameter says how we determine the width of
> +   * the nearest inflation container:  when not in reflow we look at the
> +   * frame tree; when in  reflow we look at state stored on the pres

fix double space

::: layout/forms/nsTextControlFrame.cpp
@@ +503,5 @@
>      // FIXME: This won't turn out so well for the height; maybe disable
>      // inflation entirely in this case?
>      inflation = 1.0f;
>    } else {
> +    // FIXME: THIS IS WRONG

Why? Can this be called from outside reflow?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 589948 [details] [diff] [review]
Make selects and things inside them not be containers for font size inflation, so that font inflation inflate selects appropriately along with what surrounds them.  (, patch 8)

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

Hmm, actually, what about optgroup?

This is actually a bit ugly. For example it means an <option> element outside a <select> will get special treatment. Ideally something would explicitly check if we're inside a select (or just a popup). I'm not sure what the cleanest way to do that would be...

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 590048 [details] [diff] [review]
Make floating :first-letter frames not be containers for font size inflation, to avoid triggering assertions.  (, patch 3.5)

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

I wish this wasn't getting to be more and more overhead... preface this with a check for aStyleDisplay->IsFloating()?

Comment 27

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

Attachment: Set inflation container to null during parts of intrinsic sizing that should not have inflation applied. (, patch 4)

This does as comment 21 suggests, though with a different name.

Comment 28

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

Attachment: Set inflation container to null during ComputeSize calls for inflation containers, when we should not have inflation applied. (, patch 4.5)

This is a new patch to address the FIXMEs in patch 5:  I realize that treating ComputeSize the same as intrinsic width computation will simplify that FIXME.

Comment 29

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

Attachment: Switch nsLayoutUtils inflation methods to the new setup with state on the pres context. (, patch 5)

And here's the revised version of patch 5 without those FIXMEs, on top of patch 4.5

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 590884 [details] [diff] [review]
Set inflation container to null during parts of intrinsic sizing that should not have inflation applied.  (, patch 4)

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

::: layout/base/nsLayoutUtils.h
@@ +1668,5 @@
> +      }
> +    private:
> +      const bool mIsInflationContainer;
> +      nsPresContext *mPresContext;
> +      nsIFrame *mOldValue;

You don't need the bool, you can just let mPresContext be null to indicate that you didn't set the current container to null.

Comment 31

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

Attachment: Set inflation container to null during parts of intrinsic sizing that should not have inflation applied. (, patch 4)

Updated per comment 30.

Comment 32

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

Attachment: Make selects and things inside them not be containers for font size inflation, so that font inflation inflate selects appropriately along with what surrounds them. (, patch 8)

To address the optgroup issue (comment 25), this adds one extra check for optgroup (didn't see any obvious better choice), and adds select-listbox-2 and select-combobox-2 tests to match.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Every one of these checks adds a tiny bit of overhead to the construction of most frames. And I'm not super happy about <option>s and <optgroup>s getting special treatment when outside a <select>, either.

Could nsCSSFrameConstructor detect when we're constructing frames under a <select>, and clear out the inflation-container bits for those frames?

Comment 34

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

(In reply to David Baron [:dbaron] from comment #18)
> In addition to addressing comment 17, this also adds an exception to an
> assertion for br frames, since the assertion was firing in a harmless way.

It turns out I need to make the same exception for MathML frames, for the same reason.  With that fix, try is green; now just waiting on review on the changes to patch 8.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about comment #33?

Comment 36

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

Sorry, I missed comment 33.  I suppose I should do something about the fact that I'm getting too much bugmail to actually read it and have time to do other things.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Every one of these checks adds a tiny bit of overhead to the construction of
> most frames.

See below (last comment).

> And I'm not super happy about <option>s and <optgroup>s getting
> special treatment when outside a <select>, either.

I'm not concerned about that.  I think the fact that we, internally, treat option and optgroup as 'display:block' isn't something that Web authors should depend on.  If we were honestly concerned about this we should make them display:block only when they're inside selects -- and then your concern would go away since as non-blocks outside of selects they wouldn't be inflation containers.

> Could nsCSSFrameConstructor detect when we're constructing frames under a
> <select>, and clear out the inflation-container bits for those frames?

I'm not sure this would be faster, since we'd have to set up this state on every mutation.

I could move this into a frame subclass, except that we're going to have to make this code a *lot* more complicated to fix bug 706193 -- that's going to make inflation container setting require textrun-construction-like analysis (but slightly different).  So I'd really rather not try to optimize it until during or after I work on bug 706193.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 590928 [details] [diff] [review]
Make selects and things inside them not be containers for font size inflation, so that font inflation inflate selects appropriately along with what surrounds them.  (, patch 8)

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

OK.

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/479a6867fcd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e49b2f5abc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb0f1de8252
https://hg.mozilla.org/integration/mozilla-inbound/rev/b626d3cc9ab1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3051be6f12c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8213675b8a78
https://hg.mozilla.org/integration/mozilla-inbound/rev/561d7fded0cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0cbba01ab5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3bf7112023
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bad4ac7fefe

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/479a6867fcd3
https://hg.mozilla.org/mozilla-central/rev/2e49b2f5abc1
https://hg.mozilla.org/mozilla-central/rev/bdb0f1de8252
https://hg.mozilla.org/mozilla-central/rev/b626d3cc9ab1
https://hg.mozilla.org/mozilla-central/rev/3051be6f12c2
https://hg.mozilla.org/mozilla-central/rev/8213675b8a78
https://hg.mozilla.org/mozilla-central/rev/561d7fded0cc
https://hg.mozilla.org/mozilla-central/rev/7a0cbba01ab5
https://hg.mozilla.org/mozilla-central/rev/5d3bf7112023
https://hg.mozilla.org/mozilla-central/rev/2bad4ac7fefe

Comment 40

[Tony Chung [:tchung]]

Verified fix on 1-26-2012 nightly.  Changing font size also retains the text size property in the drop down.

Comment 41

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

Comment on attachment 590928 [details] [diff] [review]
Make selects and things inside them not be containers for font size inflation, so that font inflation inflate selects appropriately along with what surrounds them.  (, patch 8)

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 42

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

[Approval Request Comment]
Regression caused by (bug #): bug 627842 (new feature)
User impact if declined: Font inflation won't apply to <select> elements.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Changes behavior of font size inflation a bit.  Shouldn't change behavior in non-DEBUG cases where font size inflation isn't enabled.

Comment 43

[Alex Keybl [:akeybl]]

Comment on attachment 590928 [details] [diff] [review]
Make selects and things inside them not be containers for font size inflation, so that font inflation inflate selects appropriately along with what surrounds them.  (, patch 8)

[Triage Comment]
Mobile only - approved for Aurora.

Comment 44

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

I'm not going to have time to land this on aurora before leaving for vacation.  It turns out it also requires landing on aurora some other font inflation patches that haven't been landed on aurora, and figuring out what those are will be a bit of work.  (The first one I hit is changeset c248c37f9cf7, but it has a merge conflict in tests which indicates there's something else under it.)  (If we're going to ship font inflation from a release based on current aurora, we'll want them... though I'm honestly not sure what our current plans are.)