224598 - [FIX] one " around cols value causes weirdness in <frameset>

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Doron Rosenberg (IBM)]

<frameset cols=13%,*">

With 2 frames, due to the lone ", the first frame takes over the whole width.

Changing the position of the lone " to the start 
<frameset cols="13%,*>

causes the left frame to take the whole width.

Have none or 2 " (ie the correct way) fixes the issue.

Minimized testcases:

- With bug    - http://www.nexgenmedia.net/evang/fotofresh/index.html
- Without bug - http://www.nexgenmedia.net/evang/fotofresh/index2.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

The first case is exactly equivalent to:

  cols='13%,*"'

while the second one is exactly equivalent to:

  cols='"13%,*'

and we should be treating them identically, which we do.  What is your suggested
rendering there?

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

If we do it for " then we should do it for ' as well.

I considered using Trim() but I chose StripChars() for values like cols=13%,*",

Comment 3

[Mats Palmgren (inactive)]

Boris, I didn't see your comment before posting the patch.  I think Doron
suggests that leading/trailing quotes should be ignored.  I think we should
just remove them everywhere.

Comment 4

[Doron Rosenberg (IBM)]

We don't treat them the same actually - depending on where the " is, the frame
we display changes.

Mats got it right, we shouldn't mess up due to an leading or trailing " - the
url of the bug shows that this is generated by PhotoShop btw.

Comment 5

[Mats Palmgren (inactive)]

I get 2 frames for '"13%,*' and 1 frame for '13%,*"'

Comment 6

[Boris Zbarsky [:bzbarsky]]

Doron, I meant that we should treat

  cols=13%,*"

the same as

  cols='13%,*"'

not that we should treat the two totally different colspecs in comment 1
identically to each other.

mats' patch is OK, I guess, if it gets lots and lots of testing... does it break
compat with IE in the cols='13%,*"' case, eg?

Comment 7

[Doron Rosenberg (IBM)]

I get 1 frame for:
<frameset frameborder="no" cols="13%,*> on trunk.

Comment 8

[Boris Zbarsky [:bzbarsky]]

doron, what does IE do with the following:

1) <frameset frameborder="no" cols="13%,*>

2) <frameset frameborder="no" cols='"13%,*'>

?

Comment 9

[Doron Rosenberg (IBM)]

1) <frameset frameborder="no" cols="13%,*>

In this case, IE goes wacky and misrenders the whole page - graphs are bleeding
into scrollbars, weird stuff.

2) <frameset frameborder="no" cols='"13%,*'>
this behaves like Mozilla, the right frame has 100%

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, well the renderings for those two cases should be absolutely identical in
Mozilla (and will be with or without Mats' patch; his patch just changes the
rendering of both cases).

You may also want to test:

1) <frameset frameborder="no" cols=13%,*">

vs

2) <frameset frameborder="no" cols='13%,*"'>

in IE.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #1: cols=13%,*"

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #2: cols='13%,*"'

Comment 13

[Mats Palmgren (inactive)]

Attachment: Testcase #3: cols="13%,*

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #4: cols='"13%,*'

Comment 15

[Mats Palmgren (inactive)]

             Mozilla  Opera7   NS4     IE5     patch1-Mozilla
Testcase #1: 100,0    13,87    100,0   13,87   13,87
Testcase #2: 100,0    13,87    100,0   13,87   13,87
Testcase #3: 0,100    0,100    0,100   --,13   0,100
Testcase #4: 13,87    0,100    50,50   0,100   13,87

numbers are "% width to left frame (google.com)" , " % to right (mozilla.org)"

Could someone please post numbers for IE6.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Testcase #3: 0,100
> Testcase #4: 13,87

I lied for the case when the quote comes first.  In testcase 3, we parse the '>'
and crud after it as part of the attr value, hence the 0,100.  In testcase 4,
that doesn't happen.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134717 [details] [diff] [review]
Patch rev. 1

Make it a single StripChars call that strips all the chars, and r+sr=bzbarsky,
since this makes us behave more compatibly.

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 19

[Boris Zbarsky [:bzbarsky]]

To Mats.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the patch, Mats!