Bug 20651 – svgElement.className.baseValue does not work

• Component: SVG
• Status: RESOLVED FIXED

[elliottcable]

In FireFox, and as I understand it, according to the standard, one doesn't simply use `.className` to set the class of an element in SVG - one must set the `.baseVal` attribute on `.className`. This works in FireFox, but apparently not in WebKit.

[elliottcable]

Created attachment 23179 [details]
Minimal test case

[elliottcable]

Forgot to mention - attached test case works in FireFox (box is green), and fails in WebKit nightly as of 4th september (box is red).

[elliottcable]

This test case now crashes WebKit when opened - here's the debugging info: http://pastie.org/266285

[Eric Seidel (no email)]

http://www.w3.org/TR/SVG/types.html#InterfaceSVGStylable

[elliottcable]

As Eric pointed out, the code (surprisingly) exhibits the correct methods for accessing/modifying the class. They just don't work d-:

Catfish_Man pointed out in IRC that the element does, in fact, show the new class in the Web Inspector DOM inspector, as well as in the Properties field - it does not, however, show the new class in the Styles pane, and the class doesn't apply in the actual document either.

[Eric Seidel (no email)]

I have a patch to fix this on my work machine.

[elliottcable]

(In reply to comment #6)
> I have a patch to fix this on my work machine.
> 

Cool, cool, looking forward to seeing it committed [-:

[Eric Seidel (no email)]

Created attachment 24124 [details]
Potential fix for element.className.baseValue

 LayoutTests/ChangeLog                              |    9 ++++
 .../svg/custom/class-baseValue-expected.checksum   |    1 +
 .../mac/svg/custom/class-baseValue-expected.png    |  Bin 0 -> 3250 bytes
 .../mac/svg/custom/class-baseValue-expected.txt    |    5 ++
 LayoutTests/svg/custom/class-baseValue.svg         |   10 +++++
 WebCore/ChangeLog                                  |   14 +++++++
 WebCore/dom/StyledElement.cpp                      |   41 ++++++++++---------
 WebCore/dom/StyledElement.h                        |    5 ++
 WebCore/svg/SVGStyledElement.cpp                   |    3 +
 9 files changed, 69 insertions(+), 19 deletions(-)

[Darin Adler]

Comment on attachment 24124 [details]
Potential fix for element.className.baseValue

Can the test be changed into a dumpAsText test? I was able to do a number of SVG tests that were portable that way.

+ if (attrName == HTMLNames::classAttr)

I'm surprised you chose to explicitly qualify here rather than doing using namespace HTMLNames.

StyledElement::classAttributeChanged would be better if it only called setChanged when there was an actual change. But that's not part of this fix, I guess.

r=me

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 24124 [details] [edit])
> Can the test be changed into a dumpAsText test? I was able to do a number of
> SVG tests that were portable that way.

I didn't do so, but it probably could have been.  :(  Honestly I somehow missed this line of your comment before commiting

> + if (attrName == HTMLNames::classAttr)
>
> I'm surprised you chose to explicitly qualify here rather than doing using
> namespace HTMLNames.

This was intentional, given that we're in an SVG file.
 
> StyledElement::classAttributeChanged would be better if it only called
> setChanged when there was an actual change. But that's not part of this fix, I
> guess.

I built a test which toggled classNames on an element to equivilent versions, I didn't see extra redraws, but we might have been doing extra style resolutions?  Or maybe my test was busted.

<style>
.foo {
    background-color: green;
}
.bar {
    width: 100px;
    height: 100px;
}
</style>
<div id="test" class="foo bar"></div>
<script>
var toggleCount = 0;
function toggleClassNames() {
    var div = document.getElementById("test");
    if (toggleCount % 2) {
        div.className = "foo bar";
    } else {
        div.className = " bar foo   ";
    }
    setTimeout(100, toggleClassNames);
}
toggleClassNames();
</script>

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/platform/mac/svg/custom/class-baseValue-expected.checksum
	A	LayoutTests/platform/mac/svg/custom/class-baseValue-expected.png
	A	LayoutTests/platform/mac/svg/custom/class-baseValue-expected.txt
	A	LayoutTests/svg/custom/class-baseValue.svg
	M	WebCore/ChangeLog
	M	WebCore/dom/StyledElement.cpp
	M	WebCore/dom/StyledElement.h
	M	WebCore/svg/SVGStyledElement.cpp
Committed r37391

[Eric Seidel (no email)]

Here is a fully functioning test case which still demonstrates that setChanged() doesn't cause a redraw and thus doesn't seem to be a big worry here re: classes being set to the same value causing too much work.

<style>
.foo {
    background-color: green;
}
.bar {
    width: 100px;
    height: 100px;
}
.baz {
    background-color: red;
}
</style>
<div id="test" class="foo bar"></div>
<script>
var toggleCount = 0;
function toggleClassNames() {
    var div = document.getElementById("test");
    if (toggleCount++ % 2) {
        div.className = "foo bar";
    } else {
        div.className = " bar  foo ";
    }
    setTimeout("toggleClassNames()", 100);
}
toggleClassNames();
</script>

[Mark Rowe (bdash)]

Rolled out in r37401 since it introduced 46 regression test failures.

[elliottcable]

Damn, and I was excited to see it in tomorrow's nightly d-:

[Eric Seidel (no email)]

Created attachment 24164 [details]
Fix the regression and avoid double-parse

 WebCore/svg/SVGStyledElement.cpp |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

Created attachment 24167 [details]
        Forgot to check in these changes in my last checkin.


        * rendering/RenderThemeSafari.cpp:



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29265 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                       |    6 ++++++
 WebCore/rendering/RenderThemeSafari.cpp |   17 +++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

Created attachment 24168 [details]
WebCore:


        Reviewed by Sam Weinig

        Fixes: http://bugs.webkit.org/show_bug.cgi?id=16523
        <rdar://problem/5657447>

        When a frame is created with the URL "about:blank" or "", it should
        inherit its SecurityOrigin from its opener.  However, once it has
        decided on that SecurityOrigin, it should not change its mind.
        Prior to this patch, several events could induce the frame to change
        its SecurityOrigin, permitting an attacker to inject script into an
        arbitrary SecurityOrigin.

        This patch makes several changes:

        1) Documents refuse to change from one SecurityOrigin to another
           unless explicitly instructed to do so.

        2) Navigating to a JavaScript URL that produces a value
           preserves the current SecurityOrigin explicitly instead of
           relying on the URL to preserve the origin (which fails for
           about:blank URLs and SecurityOrigins with document.domain set).

           Ideally, we should not preserve the URL at all.  Instead, the
           frame's URL should be the JavaScript URL, as in Firefox, but this
           would require changes that are too risky for this patch.  I'll
           file this as a separate issue.

        3) Various methods of navigating to JavaScript URLs were not
           properly handling JavaScript that returned a value (and should
           therefore replace the current document).  This patch unifies
           those code paths with the path that works.

           There are still a handful of bugs relating to the handling of
           JavaScript URLs, but I'll file those as separate issues.

        Tests: http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write.html
               http/tests/security/aboutBlank/xss-DENIED-navigate-opener-javascript-url.html
               http/tests/security/aboutBlank/xss-DENIED-set-opener.html

        * dom/Document.cpp:
        (WebCore::Document::initSecurityOrigin):
        * dom/Document.h:
        (WebCore::Document::setSecurityOrigin):
        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::changeLocation):
        (WebCore::FrameLoader::urlSelected):
        (WebCore::FrameLoader::requestFrame):
        (WebCore::FrameLoader::submitForm):
        (WebCore::FrameLoader::executeIfJavaScriptURL):
        (WebCore::FrameLoader::begin):
        * loader/FrameLoader.h:
        * platform/SecurityOrigin.cpp:
        (WebCore::SecurityOrigin::setForURL):
        (WebCore::SecurityOrigin::createForFrame):
        * platform/SecurityOrigin.h:

LayoutTests:

        Reviewed by Sam Weinig.

        Fixes: http://bugs.webkit.org/show_bug.cgi?id=16523

        Adds new LayoutTests for scripting from about:blank windows.  These
        windows should inherit its SecurityOrigin from its opener and should
        refuse to change their origins when their opener changes exogenously
        (the navigate-opener tests) or explicitly (the set-opener test).

        * http/tests/security/aboutBlank: Added.
        * http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write-expected.txt: Added.
        * http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write.html: Added.
        * http/tests/security/aboutBlank/xss-DENIED-navigate-opener-javascript-url-expected.txt: Added.
        * http/tests/security/aboutBlank/xss-DENIED-navigate-opener-javascript-url.html: Added.
        * http/tests/security/aboutBlank/xss-DENIED-set-opener-expected.txt: Added.
        * http/tests/security/aboutBlank/xss-DENIED-set-opener.html: Added.
        * http/tests/security/resources/innocent-victim-with-notify.html: Added.
        * http/tests/security/resources/innocent-victim.html: Added.
        * http/tests/security/resources/libwrapjs.js: Added.
        * http/tests/security/resources/open-window.html: Added.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29266 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 LayoutTests/ChangeLog                              |   23 ++++
 ...IED-navigate-opener-document-write-expected.txt |   17 +++
 .../xss-DENIED-navigate-opener-document-write.html |  105 +++++++++++++++++++
 ...IED-navigate-opener-javascript-url-expected.txt |   17 +++
 .../xss-DENIED-navigate-opener-javascript-url.html |  106 ++++++++++++++++++++
 .../aboutBlank/xss-DENIED-set-opener-expected.txt  |   20 ++++
 .../security/aboutBlank/xss-DENIED-set-opener.html |   76 ++++++++++++++
 .../resources/innocent-victim-with-notify.html     |   14 +++
 .../tests/security/resources/innocent-victim.html  |    5 +
 .../http/tests/security/resources/libwrapjs.js     |   62 ++++++++++++
 .../http/tests/security/resources/open-window.html |   22 ++++
 LayoutTests/platform/win/Skipped                   |    3 +
 WebCore/ChangeLog                                  |   58 +++++++++++
 WebCore/dom/Document.cpp                           |    3 +
 WebCore/dom/Document.h                             |    5 +
 WebCore/loader/FrameLoader.cpp                     |   53 +++++-----
 WebCore/loader/FrameLoader.h                       |    8 +-
 WebCore/platform/SecurityOrigin.cpp                |   55 ++++------
 WebCore/platform/SecurityOrigin.h                  |    7 +-
 19 files changed, 592 insertions(+), 67 deletions(-)

[Eric Seidel (no email)]

Created attachment 24169 [details]
        Build fix.


        * platform/graphics/qt/SimpleFontDataQt.cpp:
        (WebCore::SimpleFontData::SimpleFontData):
        (WebCore::SimpleFontData::~SimpleFontData):



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29267 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                                 |    8 ++++++++
 WebCore/platform/graphics/qt/SimpleFontDataQt.cpp |    4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

[Eric Seidel (no email)]

Created attachment 24170 [details]
Reviewed by Mark.

Enable SVG_FONTS by default.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29268 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 JavaScriptCore/ChangeLog                           |    8 ++++++++
 .../Configurations/JavaScriptCore.xcconfig         |    2 +-
 WebCore/ChangeLog                                  |    9 +++++++++
 WebCore/Configurations/WebCore.xcconfig            |    2 +-
 WebCore/WebCore.vcproj/build-generated-files.sh    |    2 +-
 WebKit/mac/ChangeLog                               |    8 ++++++++
 WebKit/mac/Configurations/WebKit.xcconfig          |    2 +-
 7 files changed, 29 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

Created attachment 24171 [details]
        Build fix.  Need to wrap these classes in #if ENABLE(VIDEO)


        * rendering/MediaControlElements.cpp:
        * rendering/MediaControlElements.h:



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29269 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                          |    7 +++++++
 WebCore/rendering/MediaControlElements.cpp |    4 ++++
 WebCore/rendering/MediaControlElements.h   |    4 +++-
 3 files changed, 14 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Created attachment 24172 [details]
2008-01-07  Jon Honeycutt  <[e-mail]>


        Reviewed by Hyatt.

        <rdar://problem/5673489> Safari does not render windowless plugins in an
        iframe when opacity < 1.0

        Plugins in transparency layers handle their own world transforms, so
        only apply the horizontal/vertical transform if we are not in a
        transparency layer.

        * platform/graphics/GraphicsContext.h: Add a Windows-platform-only
        inTransparencyLayer() function
        * platform/win/GraphicsContextWin.cpp:
        (WebCore::GraphicsContext::getWindowsContext): Use inTransparencyLayer()
        (WebCore::GraphicsContext::inTransparencyLayer):
        (WebCore::GraphicsContext::releaseWindowsContext): Use
        inTransparencyLayer()
        * plugins/win/PluginViewWin.cpp:
        (WebCore::PluginViewWin::paint): When retrieving the HDC, use the rect
        relative to the window. Pass m_isTransparent to
        get/releaseWindowsContext(). Only set the world transform if we are not
        in a transparency layer.




git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29270 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                           |   24 ++++++++++++++++++++++++
 WebCore/platform/graphics/GraphicsContext.h |    1 +
 WebCore/platform/win/GraphicsContextWin.cpp |    6 ++++--
 WebCore/plugins/win/PluginViewWin.cpp       |   27 +++++++++++++--------------
 4 files changed, 42 insertions(+), 16 deletions(-)

[Eric Seidel (no email)]

Created attachment 24173 [details]
        Add missing newline.


        * rendering/MediaControlElements.cpp:



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29271 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                          |    6 ++++++
 WebCore/rendering/MediaControlElements.cpp |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Created attachment 24174 [details]
        Build fix.


        * WebKit.vcproj/InterfacesGenerated.vcproj:



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29272 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebKit/win/ChangeLog                               |    6 ++++++
 .../win/WebKit.vcproj/InterfacesGenerated.vcproj   |    4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

[Eric Seidel (no email)]

Created attachment 24175 [details]
        Build fix.


        * platform/graphics/qt/GlyphPageTreeNodeQt.cpp:
        (WebCore::GlyphPageTreeNode::pruneTreeCustomFontData):



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29273 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                                  |    7 +++++++
 .../platform/graphics/qt/GlyphPageTreeNodeQt.cpp   |    2 +-
 2 files changed, 8 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Created attachment 24176 [details]
Fix painting of SVG <image> when the image must be scaled to retain aspect ratio


Reviewed by Niko

Also added new layout test for this bug, and corrected old expected output


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29275 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 LayoutTests/ChangeLog                              |   25 ++++++++++++++++++++
 .../animate-elem-30-t-expected.checksum            |    2 +-
 .../svg/W3C-SVG-1.1/animate-elem-30-t-expected.png |  Bin 20612 -> 20691 bytes
 .../animate-elem-36-t-expected.checksum            |    2 +-
 .../svg/W3C-SVG-1.1/animate-elem-36-t-expected.png |  Bin 32491 -> 32406 bytes
 .../animate-elem-39-t-expected.checksum            |    2 +-
 .../svg/W3C-SVG-1.1/animate-elem-39-t-expected.png |  Bin 37743 -> 38282 bytes
 .../svg/carto.net/selectionlist-expected.checksum  |    2 +-
 .../mac/svg/carto.net/selectionlist-expected.png   |  Bin 208607 -> 208824 bytes
 ...age-with-aspect-ratio-stretch-expected.checksum |    1 +
 .../image-with-aspect-ratio-stretch-expected.png   |  Bin 0 -> 3250 bytes
 .../image-with-aspect-ratio-stretch-expected.txt   |   11 ++++++++
 .../custom/pointer-events-image-expected.checksum  |    2 +-
 .../svg/custom/pointer-events-image-expected.png   |  Bin 71136 -> 68766 bytes
 .../custom/text-image-opacity-expected.checksum    |    2 +-
 .../mac/svg/custom/text-image-opacity-expected.png |  Bin 17588 -> 17393 bytes
 .../svg/custom/image-with-aspect-ratio-stretch.svg |   17 +++++++++++++
 WebCore/ChangeLog                                  |   11 ++++++++
 WebCore/rendering/RenderSVGImage.cpp               |   16 ++++++------
 19 files changed, 79 insertions(+), 14 deletions(-)

[Eric Seidel (no email)]

Created attachment 24177 [details]
        Temporary results until I fix:

        <rdar://problem/5674667> fast/forms/slider-mouse-events.html is broken by media control checkin 29257

        * fast/forms/slider-mouse-events-expected.txt:



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29276 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 LayoutTests/ChangeLog                              |    7 +++++++
 .../fast/forms/slider-mouse-events-expected.txt    |    1 -
 2 files changed, 7 insertions(+), 1 deletions(-)

[Eric Seidel (no email)]

Created attachment 24178 [details]
        * platform/win/Skipped:

        removing fixed test



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29277 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 LayoutTests/ChangeLog            |    5 +++++
 LayoutTests/platform/win/Skipped |    3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

[Eric Seidel (no email)]

Created attachment 24179 [details]
        Build fix.


        * WebCore.vcproj/WebCore.sln:
        * WebCore.vcproj/WebCore.submit.sln:



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@29278 268f45cc-cd09-0410-ab3c-d52691b4dbfc
---
 WebCore/ChangeLog                         |    7 +++++++
 WebCore/WebCore.vcproj/WebCore.sln        |    9 +++++++++
 WebCore/WebCore.vcproj/WebCore.submit.sln |    9 +++++++++
 3 files changed, 25 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 24180 [details]
Full fix (no layout test regressions)

I am *so so so* sorry for the list spam.  git-send-bugzilla freaked out a bit. :(

[elliottcable]

(In reply to comment #29)
> Created an attachment (id=24180) [edit]
> Full fix (no layout test regressions)
> 
> I am *so so so* sorry for the list spam.  git-send-bugzilla freaked out a bit.
> :(
> 

I was gonna say, this doesn't exactly look related to the bug in question... hah! <3 git though (-:

[Nikolas Zimmermann]

Comment on attachment 24180 [details]
Full fix (no layout test regressions)

Oops, good catch :-) r=me. .oO(I thought you ran layout tests before ;-)

[Eric Seidel (no email)]

I thought I did to.  Clearly I was wrong.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/37431