Form button input elements lacking text in some cases after switching from visibility:collapse to visibility:visible

• Component: Forms
• Status: RESOLVED FIXED

[Jon Watte]

1) surf on over to http://www.enchantedage.com/posbug.php
2) click a cell in the sheet
3) watch in amazement as the buttons lack their text

works fine in FF and IE (including mobile IE)

[Alexey Proskuryakov]

Created attachment 54214 [details]
reduced test case

[Ben Murdoch]

I have a patch to fix this issue.

As far as I can tell there is no support for the visibility:collapse property in WebKit. However, the spec says that elements with visibility:collapse that are not table row, row group, column or column groups should treat it as visibility:hidden. So my fix is to rewrite the collapse rule to hidden when it's not one of these table elements. This fixes this case as the element in question with visibility:collapse is a DIV.

Patch and a layout test (based on Alexey's reduced test case) to follow.

[Ben Murdoch]

Created attachment 58014 [details]
Path and layout test

Comment on attachment 58014 [details]
Path and layout test

This will break computed style and inheritance, and while the spec mentions elements, I’m pretty sure collapse should have the same effect on anything with disaply: table-*. This bug is probably a bug in layout code and should be fixed there.

[Ben Murdoch]

(In reply to comment #4)
> (From update of attachment 58014 [details])
> This will break computed style and inheritance, and while the spec mentions elements, I’m pretty sure collapse should have the same effect on anything with disaply: table-*. This bug is probably a bug in layout code and should be fixed there.

Thanks for the comments, it did seem too easy :) I'd like to delve deeper into the layout code to try and figure this one out, do you have any inkling as to where would be a good place to start looking?

[Ben Murdoch]

Spent a bit of time this afternoon and have found out the following:

When you change the visibility of an element, we treat this as only needing to do a repaint (see http://trac.webkit.org/browser/trunk/WebCore/rendering/style/RenderStyle.cpp#L509). When going from visible to hidden and vice versa, this is enough.

However in the initial layout we seem to skip elements that have visibility collapse (see http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderFlexibleBox.cpp#L93). So (I think) when we come to do the repaint triggered by the change from visible to collapse we still skip them as we don't rerun a layout.

This seems to be confirmed by triggering a layout after swapping from collapse to visible - the bug is fixed. I added

    if (inherited_flags._visibility == COLLAPSE && other->inherited_flags._visibility == VISIBLE)
        return StyleDifferenceLayout;

to RenderStyle::diff. What do you think?

Please forgive me if I'm talking nonsense. I'm not very familiar with the layout code (although I'd like to learn it) but would like to see this bug fixed.

Thanks, Ben

(In reply to comment #6)

> This seems to be confirmed by triggering a layout after swapping from collapse to visible - the bug is fixed. I added
> 
>     if (inherited_flags._visibility == COLLAPSE && other->inherited_flags._visibility == VISIBLE)
>         return StyleDifferenceLayout;
> 
> to RenderStyle::diff. What do you think?

Seems okay, but doesn’t the same problem exist when changing from HIDDEN to COLLAPSE? Also, it’s suboptimal to force layout for all the other elements, which don’t distinguish between HIDDEN and COLLAPSE, just to support flex boxes (and in the future, table parts). Not sure if that’s a serious concern, though.

[Ben Murdoch]

(In reply to comment #7)
> Seems okay, but doesn’t the same problem exist when changing from HIDDEN to COLLAPSE?

Yes, good point ;)

> Also, it’s suboptimal to force layout for all the other elements, which don’t distinguish between HIDDEN and COLLAPSE, just to support flex boxes (and in the future, table parts). Not sure if that’s a serious concern, though.

I think I can work around that by moving the code from RenderStyle::diff to RenderObject::adjustStyleDifference and only set the layout hint if it's a flex box (and add a comment to say that it should also apply to table parts in the future).

[Ben Murdoch]

Created attachment 58141 [details]
Patch and layout test v2

Here's a patch doing what I suggested above. WDYT?

Cheers, Ben

[Ben Murdoch]

Ping, thoughts anyone?

[Ben Murdoch]

Given the recent discussion on webkit-dev regarding old patches, I though I'd ping this one again. I tried applying the current patch and despite being six weeks old it still seems to apply and build fine.

Any comments folks?

Thanks, Ben

[Simon Fraser (smfr)]

Comment on attachment 58141 [details]
Patch and layout test v2

This is an OK way to do it, but rather than complexificate adjustStyleDifference(), you could just move this code to RenderStyle::diff(), and use display() == BOX to detect whether you're dealing with a flexbox.

Be sure to add your code between the last 'return StyleDifferenceLayout' and the first 'return StyleDifferenceRepaintLayer'.

[Ben Murdoch]

(In reply to comment #12)
Hi Simon,

Thanks for the comments and suggestions!

> (From update of attachment 58141 [details])
> This is an OK way to do it, but rather than complexificate adjustStyleDifference(), you could just move this code to RenderStyle::diff(), and use display() == BOX to detect whether you're dealing with a flexbox.

Hmm, display() == BOX doesn't seem to be working :( I've moved the code into RenderStyle::diff() and it works without that check. But as Mitz pointed out earlier in comment 7, it would be better to only force a layout when we are dealing with flexboxes. Any other suggestions as to how I could detect a flexbox? Or is this infact not such a big concern?

> 
> Be sure to add your code between the last 'return StyleDifferenceLayout' and the first 'return StyleDifferenceRepaintLayer'.

Will do!

Thanks, Ben

[Ben Murdoch]

I was able to hook up the debugger today to look a little closer at this.

It seems that the test page attached to the bug uses a RenderButton, which is a FlexibleBox. It inherits the visibility:collapse from it's containing <div> which I think, is not a FlexibleBox.

When we are calculating the style diff for the RenderButton, its display property is set to INLINE_BLOCK. So it doesn't seem to follow that RenderObject::isFlexibleBox() corresponds to a display of BOX. Is that correct?

I'm now starting to wonder whether this should be restricted to flexible boxes as Mitz suggested in comment 7. Should we be doing this for all RenderObjects?

[Ben Murdoch]

Created attachment 63455 [details]
Patch and layout test

[Darin Adler]

Comment on attachment 63455 [details]
Patch and layout test

> +    if ((visibility() == COLLAPSE && other->visibility() != COLLAPSE)
> +        || (visibility() != COLLAPSE && other->visibility() == COLLAPSE))
> +        return StyleDifferenceLayout;

I think it is more readable like this:

    if ((visibility() == COLLAPSE) != (other->visibility() == COLLAPSE))
        return StyleDifferenceLayout;

[Ben Murdoch]

(In reply to comment #16)
> (From update of attachment 63455 [details])
> > +    if ((visibility() == COLLAPSE && other->visibility() != COLLAPSE)
> > +        || (visibility() != COLLAPSE && other->visibility() == COLLAPSE))
> > +        return StyleDifferenceLayout;
> 
> I think it is more readable like this:
> 
>     if ((visibility() == COLLAPSE) != (other->visibility() == COLLAPSE))
>         return StyleDifferenceLayout;

SGTM, will send a replacement patch.

[Ben Murdoch]

Created attachment 63597 [details]
Patch and layout test

(In reply to comment #14)
> I was able to hook up the debugger today to look a little closer at this.
> 
> It seems that the test page attached to the bug uses a RenderButton, which is a FlexibleBox. It inherits the visibility:collapse from it's containing <div> which I think, is not a FlexibleBox.
> 
> When we are calculating the style diff for the RenderButton, its display property is set to INLINE_BLOCK. So it doesn't seem to follow that RenderObject::isFlexibleBox() corresponds to a display of BOX. Is that correct?

You’re right. RenderButton inherits from RenderFlexibleBox, and thus returns true from isFlexibleBox() regardless of its display type.

> I'm now starting to wonder whether this should be restricted to flexible boxes as Mitz suggested in comment 7. Should we be doing this for all RenderObjects?

It would still be good to do this only for flexible box, but RenderStyle alone can’t tell you whether you’re a flexible box because of the above.

[Ben Murdoch]

(In reply to comment #19)
> 
> It would still be good to do this only for flexible box, but RenderStyle alone can’t tell you whether you’re a flexible box because of the above.

Exactly, which is why I moved it into RenderObject::adjustStyleDifference in patch #2. But Simon r-'d it, suggesting I moved it back to RenderStyle::diff. Which I did in the most recent patch :).

If not RenderStyle::diff and not RenderObject::adjustSTyleDifference, do you know where else I could move the code?

Thanks!

[Ben Murdoch]

Comment on attachment 63597 [details]
Patch and layout test

Thanks :)

[Darin Adler]

Simon, Dan, maybe we still should change this back so it doesn’t affect objects that are not flexible boxes?

[WebKit Commit Bot]

Comment on attachment 63597 [details]
Patch and layout test

Clearing flags on attachment: 63597

Committed r64819: <http://trac.webkit.org/changeset/64819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #22)
> Simon, Dan, maybe we still should change this back so it doesn’t affect objects that are not flexible boxes?

I asked Simon about this and he said that 'collapse' is rarely used, so this doesn’t matter much. I agree that it would be nice to get tighten this up, but I think the patch as landed is okay too.