Bug 16270 – modifying embed.src or object.data does not work (affects WICD tests)

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: DOM

[Eric Seidel (no email)]

WebKit fails WICD <object> update data= attribute test

http://www.w3.org/2004/CDF/TestSuite/WICD_CDR_WP1/test-load-svg.xhtml

We never show the tiger like we should.  Not sure why.  I kinda doubt this is SVG-specific.

[Eric Seidel (no email)]

I expect this is the section of code causing trouble:

HTMLObjectElement::parseMappedAtrribute:

    } else if (attr->name() == dataAttr) {
        m_url = parseURL(val);
        if (renderer())
          m_needWidgetUpdate = true;
        if (renderer() && isImageType()) {
          if (!m_imageLoader)
              m_imageLoader.set(new HTMLImageLoader(this));
          m_imageLoader->updateFromElement();
        }
    } else if (attr->name() == classidAttr) {

[Eric Seidel (no email)]

Created attachment 17969 [details]
HTML-only single-file test case

Ok, so this is clearly not an SVG bug. :)

[Eric Seidel (no email)]

Now the next thing to test is if this is a regression.  It seems really strange that this doesn't work!

[Eric Seidel (no email)]

Fails ever since the very first nightly: r11976

[timur mehrvarz]

Looks like isImageType() is evaluated as false for referenced SVG childs.

[Andrew Wellington]

This is not a regression.

It also fails on Safari 2.0.4 (419.3), Mac OS X 10.4.9 PowerPC.

[timur mehrvarz]

> Looks like isImageType() is evaluated as false for referenced SVG childs.

This is because in HTMLObjectElement::isImageType(), document()->frame() is getting evaluated and then frame->loader()->client()->objectContentType(completedURL, m_serviceType) is not == ObjectContentImage.

[Eric Seidel (no email)]

Yeah, in the end I was wrong.  This has nothing to do with SVG.  It's still very strange that this has been broken all the way since Safari 2.04 and no one has noticed.  (Assuming that 2.04 still has the old 412 WebKit?)

[timur mehrvarz]

I was wrong too.

> Looks like isImageType() is evaluated as false for referenced SVG childs.

Which may be just fine, if new HTMLImageLoader(this) is not the right tool to load referenced SVG children here.

 

[Eric Seidel (no email)]

Created attachment 19899 [details]
html + svg test case, showing embed and object are broken

[Eric Seidel (no email)]

To be clear, this bug has nothing to do with SVG.  It just happens to affect a test suite with SVG content in it.  This bug is a very basic bug in <embed> and <object> which is caused by HTML code alone.

[Eric Seidel (no email)]

Ok, so it turns out that although m_widgetNeedsUpdate is set to true, nothing ever causes updateWidget(). Resizing the window after the page load seems to make updateWidget() get called.

[Eric Seidel (no email)]

Created attachment 19903 [details]
partial fix to the problem

 LayoutTests/ChangeLog                              |   11 +++++++
 .../dynamic-frame-load-after-layout-expected.txt   |   29 ++++++++++++++++++++
 .../frames/dynamic-frame-load-after-layout.html    |   28 +++++++++++++++++++
 WebCore/ChangeLog                                  |   15 ++++++++++
 WebCore/html/HTMLEmbedElement.cpp                  |    8 ++++-
 WebCore/html/HTMLObjectElement.cpp                 |    6 +++-
 6 files changed, 93 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

(In reply to comment #13)
> Created an attachment (id=19903) [edit]
> partial fix to the problem

I'm not sure this is the right fix, since it does not fix the WICD test case.  There again, m_needWidgetUpdate is being set, but no one is ever calling updateWidget() on the <object> element.

[Eric Seidel (no email)]

Created attachment 19908 [details]
Add more advanced test case (which <embed> still fails)

 LayoutTests/ChangeLog                              |   13 +++++
 .../dynamic-frame-load-after-layout-expected.txt   |   29 ++++++++++++
 .../frames/dynamic-frame-load-after-layout.html    |   27 +++++++++++
 .../dynamic-frame-replace-after-load-expected.txt  |   35 ++++++++++++++
 .../frames/dynamic-frame-replace-after-load.html   |   49 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   17 +++++++
 WebCore/html/HTMLEmbedElement.cpp                  |    8 ++-
 WebCore/html/HTMLObjectElement.cpp                 |    6 ++-
 8 files changed, 180 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

Created attachment 19921 [details]
Fix embed.src and object.data updating

 LayoutTests/ChangeLog                              |   13 +++++
 .../dynamic-frame-load-after-layout-expected.txt   |   29 ++++++++++++
 .../frames/dynamic-frame-load-after-layout.html    |   27 +++++++++++
 .../dynamic-frame-replace-after-load-expected.txt  |   34 ++++++++++++++
 .../frames/dynamic-frame-replace-after-load.html   |   48 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   17 +++++++
 WebCore/html/HTMLEmbedElement.cpp                  |   17 ++++++-
 WebCore/html/HTMLEmbedElement.h                    |    1 +
 WebCore/html/HTMLObjectElement.cpp                 |    6 ++-
 9 files changed, 188 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

I think my awesome change might be causing a crash.  Attaching.

[Eric Seidel (no email)]

Created attachment 19922 [details]
flash crash after change

[Eric Seidel (no email)]

Comment on attachment 19921 [details]
Fix embed.src and object.data updating

Sad.  See the attached crash log.  I think the loader is already deleted when addPlugInStreamLoader is called in that crash.

[Ahmad Saleem]

Using Safari 15.5 on macOS 12.4, I am getting expected results from both test cases (single-file and html+svg test case..).

In first (single-file), I am getting "PASS" across all browsers (Safari 15.5, Chrome Canary 105 and Firefox Nightly 103). In second, I am getting two "Green" square in all browsers.

I checked "computed" size via Inspect Element in Safari and both green squares are 100*100 in size.

Since I believe the expected behaviors are aligned with these test case across browsers, this bug can be marked as "RESOLVED CONFIGURATION CHANGED". Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/95545943>

[Ryosuke Niwa]

Nice, thanks for testing!