[webkit-reviews] review denied: [Bug 42767] [Windows] Home hey doesn't work in first DIV inside a TABLE : [Attachment 62262] fixes the bug
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 22 10:44:35 PDT 2010
Ojan Vafai <ojan at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 42767: [Windows] Home hey doesn't work in first DIV inside a TABLE
https://bugs.webkit.org/show_bug.cgi?id=42767
Attachment 62262: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=62262&action=review
------- Additional Comments from Ojan Vafai <ojan at chromium.org>
It would be nice if, in the cases where there are multiple dump calls, if each
one could be prefixed. Also, it would be good if dump took a second optional
argument for a description string. For example, I'd like the test expectations
for this test to be something like the following:
Tests whether home moves the caret to the beginning of line inside a
content-editable in an uneditable table.
Dump of markup 1:
<DIV id="test" contentEditable="true">
<#text>
</#text>
<DIV id="l1">
<#text>The caret is initially <selection-caret>here but should move to the
beginning of the line.</#text>
</DIV>
<#text>
</#text>
<DIV>
<#text>dummy text</#text>
</DIV>
<#text>
</#text>
</DIV>
Dump of markup 2:
<DIV id="test" contentEditable="true">
<#text>
</#text>
<DIV id="l1">
<#text><selection-caret>The caret is initially here but should move to the
beginning of the line.</#text>
</DIV>
<#text>
</#text>
<DIV>
<#text>dummy text</#text>
</DIV>
<#text>
</#text>
</DIV>
Where "Dump of markup *" is the default value for the optional description
string. If Markup.dump is only called once, without a description string, then
it should not print a description before dumping. Finally, if you make this
change, then rename Markup._description to Markup._test_description to avoid
confusion.
> +++ LayoutTests/resources/dump-as-markup.js (working copy)
Your indentation is off in many places in this file.
> + var markup = "";
> +
> + if (Markup._description && !Markup._container)
> + markup = Markup._description + '\n';
This should be += (imagine if a line got inserted above this one modifying
markup).
> + Markup._container.style.cssText = 'width:100%';
I know you're just modifying existing code, but this should probably be
"Markup._container.style.width = '100%';". No reason to use cssText if we're
not actually setting multiple properties.
More information about the webkit-reviews
mailing list