Bug 9217 – padding-bottom not working for <a>

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Miles Lubin]

The css style padding-bottom is apparently not followed for <a> tags. Knoyzz.com uses it in its navigation bar to create an underline-like-border under the link. On firefox and IE, the borders are well under the text, but on Safari the borders are on top of the text. 

The relevent css is at the end of http://knoyzz.com/style.css

[Alice Liu]

Created attachment 8654 [details]
screenshot of difference

screenshot of difference.  Safari on left, FF on right.

[Alice Liu]

confirming, moving to new.  added screenshot of difference, safari on left, FF on right. 

[Sam Weinig]

Created attachment 8655 [details]
reduced test case

[Dave Hyatt]

Bet this works in strict mode.

[Miles Lubin]

Created attachment 8656 [details]
strict mode

You were right, it does work correctly in strict mode.

[Dave Hyatt]

This is related to the quirk involving hasTextChildren() and shrinkBoxesWithNoTextChildren() in InlineFlowBox.cpp.  Right now we shrink any inline boxes that have no immediate text children.  We should either propagate the bit up so that an ancestor is also considered to have text children (not sure if this is right though), or we could check for a bottom border along with hasTextChildren.

[Dave Hyatt]

Firefox disables the quirk when border-bottom is set, so I think that's the way we should go too.

[Dave Hyatt]

Same with padding-bottom.  They actually turn off the quirk even if top border/padding is set, but I think that's overkill.

[Dave Hyatt]

I'll leave this to someone else to code up the fix for.   Just need to replace most of the hasTextChildren()s with a new method like shouldShrinkBox() that could check hasTextChildren(), bottom border/padding, and strict mode all together.

[David Carson]

Changed component to Layout and Rendering, as it seems to be in the layout.
InlineFlowBox.cpp does checks for strictMode || hasTextChildren(). Seams like an easy fix.

[Sam Weinig]

Created attachment 8698 [details]
patch

Makes fix and cleans up some surrounding code a little bit.

[Dave Hyatt]

Comment on attachment 8698 [details]
patch

You want to shrink the boxes if you *aren't* in strict mode.  Change the strictMode check to !strictMode.

[Miles Lubin]

If I have it correctly, we don't want to shrink the box if it's in struct mode, or if there are text children, or if there is a bottom border, or if there is bottom padding.

The expression "(strictMode || hasTextChildren() || object()->borderBottom() || object()->paddingBottom())" looks like it does the exact opposite, it will shrink if any of those are true. Putting a ! before the expression should produce the correct logic. I'm currently building webkit with this patch and will report on how it comes out.

[Miles Lubin]

my change doesn't seem to do it

[Miles Lubin]

Created attachment 8704 [details]
inlineInInline.diff

Fixed patch with hyatt's suggested change. Tested and is working properly.

[Miles Lubin]

After running the regression tests, this patch causes 31 cases to have a minor aesthetic change in their layout. I did not see any real problems, though I will look at it more tomorrow if someone else doesn't do it before then.

[Dave Hyatt]

Comment on attachment 8704 [details]
inlineInInline.diff

I see a missed spot that still needs to be patched with shouldShrinkBox...

There's an

if (hasTextChildren() || strictMode)

left in the code still.

[Miles Lubin]

Created attachment 8721 [details]
inlineInInline.diff

Fixed logic based on IRC discussion, and replaced the (hasTextChildren() || strictMode) that were left over. Compiles and appears to work as desired.

[Dave Hyatt]

Comment on attachment 8721 [details]
inlineInInline.diff

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> (From update of attachment 8721 [details] [edit])
> r=me

I saw a lot of layout failures when I applied this patch and ran run-webkit-tests.  Please run all of the layout tests before applying this patch.

[Dave Hyatt]

Comment on attachment 8721 [details]
inlineInInline.diff

I wouldn't expect to see a lot of layout test failures so something must be wrong.  Minusing.

[Adam Roben (:aroben)]

This seems to be fixed in the latest nightly. Resolving.