Bug 17767 – frameborder="no" on frameset is ignored if border attribute set

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Gavin Sherlock]

Everything on the page is shifted down, with the main content of the page being in a tiny frame on the bottom right.  Renders correctly in Firefox.  Tested with r30939 and release Safari on 10.5.2.  Have not tested if it is a regression from Safari 2.

[Marcus]

The page has two framesets with these attributes:
framespacing="0" frameborder="NO" border="800"

IE 7 prefers framespacing="0" over border="800" and draws no border between the frames.
Safari 3.1 ignores the framespacing attribute and draws a 800-pixel wide border.
Firefox 3 also ignores the framespacing attribute but interprets frameborder="NO" to signify that no border should be drawn.

[Jon@Chromium]

Also see Chromium bug http://code.google.com/p/chromium/issues/detail?id=1365

[Elliott Sprehn]

Created attachment 149383 [details]
Reduction

[Elliott Sprehn]

Created attachment 149410 [details]
Patch

[Julien Chaffraix]

Comment on attachment 149410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149410&action=review

How good is our coverage of |frameborder| and |border|? I had a hard time understanding why you wouldn't break any case: checking |m_frameborder| would normally be dependent on the attribute ordering but here works because it defaults to true. If something changes, some of cases will break which why I would like you to point out or provide more coverage for those.

> Source/WebCore/html/HTMLFrameSetElement.cpp:113
>              if (!m_border)
>                  m_frameborder = false;

This should go into the "if (m_frameborder)".

[Tony Chang]

Comment on attachment 149410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149410&action=review

> LayoutTests/fast/frames/frame-frameborder-overides-border.html:5
> +    WebKit doesn't ignore border="..." when frameborder is NO while all other
> +    browsers will.

I would probably use a dumpAsText test instead.  It's slightly easier to maintain than a reftest.  Also, for dumpAsText or reftests, I would include the description text in the generated output.  It might also be nice to add tests for frameborder={0,1,yes}.

[Tony Chang]

Ah, I didn't realize order matters.  Seems like we need more test cases!

[Elliott Sprehn]

Comment on attachment 149410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149410&action=review

>> LayoutTests/fast/frames/frame-frameborder-overides-border.html:5
>> +    browsers will.
> 
> I would probably use a dumpAsText test instead.  It's slightly easier to maintain than a reftest.  Also, for dumpAsText or reftests, I would include the description text in the generated output.  It might also be nice to add tests for frameborder={0,1,yes}.

So I should dump the borders using getComputedStyle?

[Elliott Sprehn]

(In reply to comment #7)
> Ah, I didn't realize order matters.  Seems like we need more test cases!

Yeah it works because the default is true as required by IE compatibility behavior. I'll add more tests since this might not be obvious.

[Tony Chang]

Comment on attachment 149410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149410&action=review

>>> LayoutTests/fast/frames/frame-frameborder-overides-border.html:5
>>> +    browsers will.
>> 
>> I would probably use a dumpAsText test instead.  It's slightly easier to maintain than a reftest.  Also, for dumpAsText or reftests, I would include the description text in the generated output.  It might also be nice to add tests for frameborder={0,1,yes}.
> 
> So I should dump the borders using getComputedStyle?

Yeah, that's what I was thinking.  Maybe that doesn't work for framesets or frames?  You could also get the clientWidth of the frames and make sure they're the right size.

[Elliott Sprehn]

(In reply to comment #5)
> (From update of attachment 149410 [details])
> ...
> > Source/WebCore/html/HTMLFrameSetElement.cpp:113
> >              if (!m_border)
> >                  m_frameborder = false;
> 
> This should go into the "if (m_frameborder)".

They logically have the same effect but that's more nesting. Is there a reason?

[Julien Chaffraix]

Comment on attachment 149410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149410&action=review

>>> Source/WebCore/html/HTMLFrameSetElement.cpp:113
>>>                  m_frameborder = false;
>> 
>> This should go into the "if (m_frameborder)".
> 
> They logically have the same effect but that's more nesting. Is there a reason?

I don't see what the issue with more nesting is. With the new logic, we don't need to run this check if m_frameborder is false (in which case m_border will be false anyway).

> LayoutTests/fast/frames/frame-frameborder-overides-border.html:14
> +    but instead you see the above with lots of white spaces around it because
> +    the frameborder still renders.
> +-->

Also this description should be dumped in the output if it doesn't make your test platform dependent (which is the case for ref-tests and dumpAsText). I also advise to add the bug title and number but other reviewers have other opinions on that. That makes tests whose output are easier to read and maintain.

[Elliott Sprehn]

Created attachment 150005 [details]
Patch

[Elliott Sprehn]

It turns out there was more brokeness in there like frameborder actually being parsed to an int making frameborder=yes the same as frameborder=no.

I've done a bunch of fix up on the parsing and handling of inheritance and added lots of tests.

[Elliott Sprehn]

Created attachment 150013 [details]
Patch

Fix funky whitespace

[Tony Chang]

Comment on attachment 150013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150013&action=review

> Source/WebCore/html/HTMLFrameSetElement.cpp:101
> +            } else if (equalIgnoringCase(value, "yes") || equalIgnoringCase(value, "1")) {
> +                // Default is true.

I think this comment is confusing (it's not obvious we're talking about m_border). I would just remove it.

[Elliott Sprehn]

Created attachment 150021 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 150021 [details]
Patch for landing

Clearing flags on attachment: 150021

Committed r121494: <http://trac.webkit.org/changeset/121494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.