Bug 34415 – [Qt] QWebPluginFactory based plugins are always painted on top, ignore z-index

• Component: New Bugs
• Status: RESOLVED INVALID

[Bernhard Rosenkraenzer]

In the context of

<p>This is a test...</p>
<object type="something/handled.by.the.plugin" width="120" height="120"></object>
<p>This is a test...</p>
<div id="darken" style="position:fixed;height:100%;width:100%;top:0;left:0;background:#000000;border:none;z-index:500;opacity:0.7;"/>

The 2 "This is a test..." paragraphs are covered by a the "darken" div (typical context: "darken" is set to display:none in the beginning and activated by JS when e.g. a popup is shown).
The object (something/handled.by.the.plugin), however, is not darkened -- it's drawn on top of the "darken" div despite the fact that its z-index is lower.

[Benjamin Poulain]

Could you post an example/reduction for the issue?
Are you sure you use windowless plugins?

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component

[Kent Hansen]

Closing until we receive a testcase that demonstrates the issue.

[Frank Hanamy]

Created attachment 50549 [details]
Bug demo

Confirm this bug. QWebPluginFactory based plugins are always painted on top no matter what z-index value is. I attached a small program demonstrating this issue.
Qt version: 4.6.2
Tested under Linux and Windows XP

[Kent Hansen]

Reopening thanks to Frank's testcase.

[Robert Hogan]

*** Bug 32174 has been marked as a duplicate of this bug. ***

Created attachment 82623 [details]
another example

Another example showing the ignored z-index problem.

I've just added another exmaple which shows the problem.

I got this from a customer of mine who eventually wants to have a:
QDeclarative-QWebPlugin-Video setup 

Where the video is the QWebPlugin and above that video  web-content should be displayed.


This example is using QWidgets (instead of QDeclarative) for simplicity.

[Robert Hogan]

The tall red bar is placed on top for me compiled against WebKit trunk, and I think that is the expected result. Can you provide a screenshot of what you're seeing?

I think this might have been fixed by https://bugs.webkit.org/show_bug.cgi?id=52594

[Robert Hogan]

Created attachment 82692 [details]
My result against trunk

Video plugin obeys z-ordering

I just pulled from gitorious.org/qt (master) 
and applied https://bugs.webkit.org/show_bug.cgi?id=52594

But "Blue" is still in foreground./This patch is not solving the problem 
=> another commit must be responsible.

Any ideas where/how to find the corresponding commit/patch?


(qt master head was: dee5662e0218e754d425c8960edb1e8ffd27fde6)

[Robert Hogan]

(In reply to comment #11)
> I just pulled from gitorious.org/qt (master) 
> and applied https://bugs.webkit.org/show_bug.cgi?id=52594
> 
> But "Blue" is still in foreground./This patch is not solving the problem 
> => another commit must be responsible.
> 
> Any ideas where/how to find the corresponding commit/patch?
> 
> 
> (qt master head was: dee5662e0218e754d425c8960edb1e8ffd27fde6)

Haven't the faintest. But looking at FrameLoaderClientQt.cpp, this QWebPluginWidget is getting instantiated as a QtPluginWidget. So the patch I pointed to isn't relevant.

I don't see where z-ordering is respected for QtPluginWidgets (and I believe it's not) - so I don't know why this is passing for me against trunk. So, haven't a clue basically.

Created attachment 87493 [details]
another example - updated

Errors fixed in the nokia.html

The problem is NOT fixed.

I turned out that in the first version I've uploaded there was a syntax-error.

Older versions of webkit (e.g. qtwebkit2.0) were forgiving enough and were creating and painting the "onBottom" (the blue area).

Newer versions webkit (e.g. qtwebkit2.1) weren't forgiving enough 
=> they didn't create the "onBottom" => no blue.


I've uploaded a better version were:
1. the syntax-error is fixed
2. changed the second red to green
3. changed the sizes so no mater what you should always see all 3 colors.


The right order of those colors should be:
red -> blue -> green

But with trunk@82417 (from git://gitorious.org/webkit/webkit.git) you got:
blue -> red -> green

and that's exactly this bug.

Created attachment 87533 [details]
another example - QML

The same example as "another example - updated" but based on QDeclarativeView and QGraphicsWebView as QDeclarativeItem.
The point here: The same bug occurs for QGraphicsView/QDeclarativeView.

[Girish Ramakrishnan]

Created attachment 89088 [details]
Make QtGraphicsPluginWidget AC code path friendly

[Kenneth Rohde Christiansen]

Comment on attachment 89088 [details]
Make QtGraphicsPluginWidget AC code path friendly

View in context: https://bugs.webkit.org/attachment.cgi?id=89088&action=review

Looks sensible. Any manual test? :-)

> Source/WebKit/qt/ChangeLog:10
> +        content because they were not integratede into the AC code path. The

integrated*

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1442
> -class QtPluginGraphicsWidget: public Widget
> +class QtPluginGraphicsWidget: public PluginViewBase

I still think we need to move these somewhere else :/ The classes I mean, out of FrameLoaderClient

[Girish Ramakrishnan]

I had a discussion with Benjamin on irc. He mentioned that the new texmap code that is coming up will only break this feature and hence cause a regression.

So, it was decided to not commit this even though it fixes the problem.

When texmap gets in, we have to look into a windowless API for rendering external content so that we can achieve interleaved rendering.

[Girish Ramakrishnan]

One point I would like to raise is that we keep talking of the 'hybrid' approach. And at this point, we cannot even interleave html content and native content which is very important for TV customers. IMO, with holding this patch because texmap cannot do it, doesn't seem to be the right solution. Instead, if texmap cannot do it, then we should fix texmap or have an alternate api/solution.

Just putting down my thoughts, I am fine if this patch does not get into main line :-)

[Girish Ramakrishnan]

(In reply to comment #17)
> (From update of attachment 89088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89088&action=review
> 
> Looks sensible. Any manual test? :-)
> 

Thanks for the review, Kenneth. I guess I could add the examples attached in this bugreport as manual tests. The thing was the manual test for this requires me to check-in the c++ code too under Source/WebCore/manual-tests/qt. Do you think it's worth it i.e will someone actually find it and use it?

> > Source/WebKit/qt/ChangeLog:10
> > +        content because they were not integratede into the AC code path. The
> 
> integrated*
> 

Oops, I will fix it when landing (if we agree it should be landed).

[Kenneth Rohde Christiansen]

We have autotests already, can't it be added there?

[Girish Ramakrishnan]

Quick update: I am working with the customer to verify if this fix works for them. I will upload a new patch with tests after he confirms.

[Girish Ramakrishnan]

Customer has confirmed it works. I am writing a test case now so we can try to commit it.

[Girish Ramakrishnan]

Created attachment 91318 [details]
Make QtGraphicsPluginWidget AC code path friendly (2)

[Girish Ramakrishnan]

Created attachment 91331 [details]
Make QtGraphicsPluginWidget AC code path friendly (take 3)

create patch with --binary

[Girish Ramakrishnan]

Created attachment 91333 [details]
Patch against ToT

[WebKit Review Bot]

Attachment 91333 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:32:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:40:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:57:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:64:  Declaration has space between type name and * in QObject *create  [whitespace/declaration] [3]
Source/WebKit/qt/tests/qwebpluginfactory/tst_qwebpluginfactory.cpp:70:  Declaration has space between type name and * in TestPlugin *plugin  [whitespace/declaration] [3]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Girish Ramakrishnan]

This patch doesn't work with trunk anymore. 

Last known good revision is 83789. It broke somewhere between that and 83914.

[Eric Seidel (no email)]

Comment on attachment 91333 [details]
Patch against ToT

r- since the patch supposedly doesn't work anymore.

[Viatcheslav Ostapenko]

(In reply to comment #29)
> (From update of attachment 91333 [details])
> r- since the patch supposedly doesn't work anymore.

It works (at least for me ;) ) after http://trac.webkit.org/changeset/86582
The whole AC was broken on QGraphicsWebView.

Girish, please, make sure that it works.

[Girish Ramakrishnan]

(In reply to comment #30)
> (In reply to comment #29)
> > (From update of attachment 91333 [details] [details])
> > r- since the patch supposedly doesn't work anymore.
> 
> It works (at least for me ;) ) after http://trac.webkit.org/changeset/86582
> The whole AC was broken on QGraphicsWebView.
> 
> Girish, please, make sure that it works.

Thanks for the update. I can confirm that it works fine with trunk of yesterday. Let me check with simon/torarne (here at meego conf) on how to proceed with this.

[Simon Hausmann]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > (From update of attachment 91333 [details] [details] [details])
> > > r- since the patch supposedly doesn't work anymore.
> > 
> > It works (at least for me ;) ) after http://trac.webkit.org/changeset/86582
> > The whole AC was broken on QGraphicsWebView.
> > 
> > Girish, please, make sure that it works.
> 
> Thanks for the update. I can confirm that it works fine with trunk of yesterday. Let me check with simon/torarne (here at meego conf) on how to proceed with this.

I think in the light of the progress with Noam's texture mapper and the intent to make it a good default, this patch is going to need rework :)

[Viatcheslav Ostapenko]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > (In reply to comment #29)
> > > > (From update of attachment 91333 [details] [details] [details] [details])
> > > > r- since the patch supposedly doesn't work anymore.
> > > 
> > > It works (at least for me ;) ) after http://trac.webkit.org/changeset/86582
> > > The whole AC was broken on QGraphicsWebView.
> > > 
> > > Girish, please, make sure that it works.
> > 
> > Thanks for the update. I can confirm that it works fine with trunk of yesterday. Let me check with simon/torarne (here at meego conf) on how to proceed with this.
> 
> I think in the light of the progress with Noam's texture mapper and the intent to make it a good default, this patch is going to need rework :)

I think 2.2 needs it as is.
IMHO, texture mapper plugins layer should be separate task.

[Eric Seidel (no email)]

Comment on attachment 89088 [details]
Make QtGraphicsPluginWidget AC code path friendly

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 89088 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Gauvain Pocentek]

Hi,

I was wondering if there's any progress on this bug? I still experience it with Qt 5.0.1 on linux.

Is there any information I could provide to help?

Thanks.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.