380004 - Images with "height=percentage" don't scale correctly as page is resized.

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Daniel Holbert [:dholbert]]

<img> tags with the "height=percentage" attribute don't scale correctly as the firefox window is resized vertically.  

If you click the lower-right corner of the window to perform the resize, the image scales correctly, but if you resize from the *bottom* edge of the window, the image size usually does not scale as it should.  (The only time I've seen it scale correctly in this situation is when I make the firefox window smaller than the image itself.  Then the image suddenly "snaps" smaller.)

A separate but related issue (probably should be filed as another bug) is that when the firefox window is smaller than a certain width (approx. 400 pixels), then you can't get the image to resize even by dragging the window's lower-right corner. (except by making the window's height less than the images height, as before)

I've observed this bug in quirks mode and almost-standards mode on Trunk. (doesn't occur in Firefox 2.0.0.3)

In full standards mode, the bug is not applicable because the image's "height=[percentage]" attribute seems to be ignored. (presumably because it's non-standard.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase

Here's a testcase

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Tentative Patch: Mark lines as dirty on vertical resizes (not just on horizontal)

The underlying issue was that, within the nsBlockFrame::Reflow method, PrepareResizeReflow wasn't getting called when we had a vertical resize -- it was only getting called for horizontal resizes.  Consequently, the Line that contains the Body wasn't getting marked as dirty, and so the body wasn't getting reflowed, and the image wouldn't get resized.

The original code called PrepareResizeReflow whenever there was a horizontal resize -- my proposed patch makes the call for vertical resizes, as well.

This is tentative, as I haven't run all the test suites on it yet.  dbaron, let me know if this looks like it'd break any standards or anything.  My guess is that the code was the way it was because in standards mode, maybe PrepareResizeReflow should never need to be called for vertical resizes.  However, this almost-standards-mode case is one situation where PrepareResizeReflow needs to be called. (that, or another workaround needs to be found.)  Also, if I'm right about that last bit, maybe we can improve my patch's performance by adding an additional check to see if we're in standards mode. (not sure how to do that)

Comment 3

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

I don't think this is the right approach.  PrepareResizeReflow makes decisions about what lines to mark dirty based on the idea that it's a horizontal resize -- and it may not mark the necessary lines dirty.

I wrote a bit of code to deal with what needs to be reflowed for vertical resizes -- search for NS_FRAME_CONTAINS_RELATIVE_HEIGHT.  nsHTMLReflowState::ShouldReflowAllKids should return true in the necessary cases.  But perhaps the real problem is the XXX comment in nsHTMLReflowState::InitResizeFlags?

Comment 4

[Daniel Holbert [:dholbert]]

Notes from debugging:
When I do a vertical resize, the "Body" frame has its NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit turned on, but the higher-up "Area" frame doesn't, so reflowing doesn't descend any further below the "Area" frame. (i.e. the Body and the image it contains doesn't get reflowed).

Going into more detail, the moment where this bit makes a difference is at line 1660 in nsBlockFrame.cpp, within the function nsBlockFrame::ReflowDirtyLines.  When this function is called for the "Area" frame, the "selfDirty" flag doesn't get set, because the  NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit is *off* when it should be on.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
Here's why the Area frame doesn't get its NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit turned on:

In the initial page load, nsHTMLReflowState::InitResizeFlags is called with frame == the image frame, and at line 368, we walk up the frame tree, setting the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bits as we go up.

However, that loop ends when we hit rs == mCBReflowState, which happens to be the Body's reflow state.  So, we don't go up any further than the body.  As a result, the Area frame never has its NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit set.  My understanding is that this bit *should* be set -- correct me if I'm wrong.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Tentative Patch #2: When determining if a line is dirty, check if it's got a block with relative-height children

This patch adds an additional case in which we mark lines as dirty inside of nsBlockFrame::ReflowDirtyLines.  Specifically, it marks a line as dirty if...
 - we're doing a vertical resize
 - the line is a block
 - the line contains something with NS_FRAME_CONTAINS_RELATIVE_HEIGHT

This patch fixes the issue because it makes the Line that contains the Body block be marked as dirty on vertical resizes (when nsBlockFrame::ReflowDirtyLines is run on behalf of the nsAreaFrame).

Note: I'm not 100% sure that checking line->mFirstChild is enough -- could there be more children beyond mFirstChild that might need checking?  I wrote it that way to mirror the NS_BLOCK_HAS_CLEAR_CHILDREN section immediately above my change.

Comment 7

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

Is the NS_FRAME_CONTAINS_RELATIVE_HEIGHT flag not getting propagated high enough up the tree here?  The assignment to |selfDirty| in nsBlockFrame::ReflowDirtyLines should have been sufficient to do this.  Why isn't it?

Comment 8

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

(In reply to comment #6)
> Note: I'm not 100% sure that checking line->mFirstChild is enough -- could
> there be more children beyond mFirstChild that might need checking?  I wrote it
> that way to mirror the NS_BLOCK_HAS_CLEAR_CHILDREN section immediately above my
> change.

If a line IsBlock(), then it has only one child.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> Is the NS_FRAME_CONTAINS_RELATIVE_HEIGHT flag not getting propagated high
> enough up the tree here?

Possibly -- I'm not sure what's high up enough vs. too high.  But if "body" being the top isn't high up enough, then yes.

For details, see my comment #5 -- in short, NS_FRAME_CONTAINS_RELATIVE_HEIGHT gets propagated up one level, to the Body frame, but does't get past that to the Area frame.  It stops there because of the "while (rs != mCBReflowState)" check in nsHTMLReflowState::InitResizeFlags, line 369.

Maybe that while check should be changed, or maybe it should stay the same but the mCBReflowState should be changed (for the ImageFrame) -- I don't totally know the semantics of what mCBReflowState represents, so I'm not sure.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> The assignment to |selfDirty| in
> nsBlockFrame::ReflowDirtyLines should have been sufficient to do this.  Why
> isn't it?

selfDirty is *not* set when ReflowDirtyLines is called on the Area frame.  (because the Area frame doesn't have NS_FRAME_CONTAINS_RELATIVE_HEIGHT turned on.)

On the other hand, selfDirty *is* set when ReflowDirtyLines is run on the Body frame, because the Body has NS_FRAME_CONTAINS_RELATIVE_HEIGHT turned on.  However, on a vertical resize, ReflowDirtyLines never even gets called on the Body frame, because we get blocked at the Area frame.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Patch: Match behavior of CalcQuirkContainingBlockHeight when propagating the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit

When we're running in quirks mode, this patch makes us propagate the  NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit up as high as CalcQuirkContainingBlockHeight does.

Comment 12

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

Comment on attachment 264769 [details] [diff] [review]
Patch: Match behavior of CalcQuirkContainingBlockHeight when propagating the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit

>+ * Could probably be used within CalcQuirkContainingBlockHeight, but
>+ * I'm not sure how to do this cleanly, given that 
>+ * CalcQuirkContainingBlockHeight does some work (changing variables)
>+ * between its two continue statements.

"I'm not sure how to do this cleanly" tends not to make a whole lot of sense out of the context of the patch.  I'd suggest replacing this with "XXX Maybe refactor CalcQuirkContainingBlockHeight so it uses this function as well"

>+PRBool
>+IsQuirkContainingBlockHeightRoot(const nsHTMLReflowState* rs) {

Should probably be static (which makes the name file-scope and the linkage internal), as should CalcQuirkContainingBlockHeight itself.

"Root" probably isn't the best term; you're looking for things that are the basis for a percentage height.  Perhaps IsQuirkContainingBlockHeight, or IsQuirkPercentHeightBasis?

Local style says the { for a function should be on its own line.

>+        return false;
>+        return false;
>+  return true;

PR_FALSE and PR_TRUE


I realize we should probably add a check for NS_FRAME_REFLOW_ROOT in both functions, since if we're walking the reflow state chain rather than the frame tree, crossing a frame with NS_FRAME_REFLOW_ROOT could lead to indeterministic behavior since the reflow state chain may or may not continue above it, depending on why we're doing the reflow.  But that should probably be filed as a separate bug.

I still need to look at the details...

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Updated patch, RE dbaron's comments

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 264787 [details] [diff] [review]
Updated patch, RE dbaron's comments

Added dbaron's suggestions.

Also restructured to make sure that we get at least as far as mCBReflowState in the quirks-mode case.  (The (IsQuirkContainingBlockHeight check doesn't ensure that, because CalcQuirkContainingBlockHeight is usually called *on* the containing block, whereas here, we're walking up the chain starting a bit lower)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: More-updated patch

Added a few more changes suggested by dbaron, and added some comments.

Also made CalcQuirkContainingBlockHeight static, and removed an unnecessary check for rs->frame within that function.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Prev patch with a reftest included

Comment 17

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

Comment on attachment 264898 [details] [diff] [review]
Prev patch with a reftest included

>+ * XXX Maybe refactor CalcQuirkContainingBlockHeight so it uses this function as well

Could you wrap this at less than 80 characters?

>     const nsHTMLReflowState *rs = this;
>+    PRBool hitCBReflowState = false;

PR_FALSE

>+	myFrame = document.getElementById("myFrame");

You probably want "var myFrame"; without it, you're setting a global variable.

>Index: reftests/bugs/380004-embed.html

I'd prefer not to call the file -embed, since embed is the name of an HTML element.  How about -inner or -iframe?  (iframe's an appropriate HTML element, since this is the contents of an IFRAME element.)

r+sr=dbaron with that fixed.

If you attach a new patch with those fixed, I can land it for you.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: Patch (incorporated dbaron's last suggestions)

Thanks for the suggestions -- here's the updated patch.

Comment 19

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

Comment on attachment 264917 [details] [diff] [review]
Patch (incorporated dbaron's last suggestions)

I landed this on the trunk, 2007-05-15 14:53 -0700.