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.
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.
Also see Chromium bug http://code.google.com/p/chromium/issues/detail?id=1365
Created attachment 149383 [details] Reduction
Created attachment 149410 [details] Patch
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)".
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}.
Ah, I didn't realize order matters. Seems like we need more test cases!
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?
(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.
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.
(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?
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.
Created attachment 150005 [details] Patch
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.
Created attachment 150013 [details] Patch Fix funky whitespace
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.
Created attachment 150021 [details] Patch for landing
Comment on attachment 150021 [details] Patch for landing Clearing flags on attachment: 150021 Committed r121494: <http://trac.webkit.org/changeset/121494>
All reviewed patches have been landed. Closing bug.