386065 - legend {font-size:0;} still shows the text (yahoo.com mainpage)

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[u88484]

Attachment: Screenshot

On www.yahoo.com there is a <legend> form control for the search box that is showing up even with css enabled (I understand the legend tag as not supposed to show if forms are rendered in a graphical manner).  See next attachments for screenshot and reduced code.  I probably did not title the bug correctly so forgive me.

I'm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070626 Minefield/3.0a6pre and started noticing this a few days ago.  Will check to see if its regression and the range if so.

Comment 1

[u88484]

Attachment: reduced testcase

Comment 2

[u88484]

This regressed between 2007-06-20-04 and 2007-06-21-07, I'm guessing Bug 367177 – Turn on nsTextFrameThebes caused this.  I suck at using the tinderbox to find regression windows so please forgive me for not narrowing it down even more.

Comment 3

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

I think the old text frame had specific checks for 0 font size.  That said, I was never happy about them, since they encourage Web sites to use tricks that are broken by the minimum font size pref.  (Then again, we could make the minimum font size pref work better and not change 0.)

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

What are we aiming for here? Should the behaviour match FF2, i.e. in the test case should there be a gap of white space in the top of the fieldset's border box? (This is the invisible size-0 legend text's padding/margin). Or should the textframe vanish, making the fieldset outline unbroken, as if there was no legend text?

[Taking]

Comment 5

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

I think we should still have the gap. This is not about changing the behaviour of fieldset, it's just about making size 0 text not display anything.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - allow font-size:0, create gfxTextRun with Missing glyphs

This patches MakeTextRun() in layout/generic/nsTextFrameThebes.cpp so that if it detects its encountering text which is font-size 0, it will make a text run which contains the characters which have been SetMissing(). This way text runs with font-size 0 are omitted from metric calculations and painting.

I also needed to change gfxTextRun::FindFirstGlyphRunContaining() slightly to prevent an assertion firing, as the gfxTextRun would have an empty mGlyphRuns when all its glyphs as missing.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - Previous patch refactored

This is similar to the previous, but has some refactorings based on suggestions by ROC:

* Moved MakeMissingGlyphsTextRun() into gfxTextRunWordCache.cpp.
* MakeMissingGlyphsTextRun() now adds a glyph run to the new text run.
* Removed SetAllGlyphsMissing(); it was not needed.

Comment 8

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

Comment on attachment 284412 [details] [diff] [review]
Patch v2 - Previous patch refactored

This is fine, just a cuople of things could be better:

-- MakeMissingGlyphsTextRun would be better named MakeBlankTextRun, just to be clear that we're not going to show the "hexbox" fallback we would normally show for missing glyphs

-- You should probably document somewhere that we're short-circuiting for size 0 fonts because Windows and ATSUI, at least, can't handle a size 0 request properly.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - with ROC's latest suggestions.

Carrying over review from previous patch, only minor style changes as per ROC's suggestions from Comment 8.

Comment 10

[Reed Loden [:reed]]

Checking in gfx/src/thebes/nsThebesFontMetrics.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesFontMetrics.cpp,v  <--  nsThebesFontMetrics.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in gfx/thebes/src/gfxTextRunWordCache.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxTextRunWordCache.cpp,v  <--  gfxTextRunWordCache.cpp
new revision: 1.11; previous revision: 1.10
done

Comment 11

[u88484]

Verified that the text no longer shows on yahoo.com.

Comment 12

[Jesse Ruderman]

Attachment: reftests (checked in)