Bug 34415 - [Qt] QWebPluginFactory based plugins are always painted on top, ignore z-index
Summary: [Qt] QWebPluginFactory based plugins are always painted on top, ignore z-index
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P3 Normal
Assignee: Girish Ramakrishnan
URL:
Keywords: Qt, QtTriaged
: 32174 (view as bug list)
Depends on:
Blocks: 57418
  Show dependency treegraph
 
Reported: 2010-02-01 03:13 PST by Bernhard Rosenkraenzer
Modified: 2014-02-03 03:16 PST (History)
11 users (show)

See Also:


Attachments
Bug demo (1.93 KB, application/gzip)
2010-03-11 15:16 PST, Frank Hanamy
no flags Details
another example (2.38 KB, application/octet-stream)
2011-02-16 06:18 PST, thomas.senyk
no flags Details
My result against trunk (15.67 KB, image/png)
2011-02-16 14:13 PST, Robert Hogan
no flags Details
another example - updated (2.38 KB, application/x-gzip)
2011-03-30 03:15 PDT, thomas.senyk
no flags Details
another example - QML (2.41 KB, application/x-gzip)
2011-03-30 07:48 PDT, thomas.senyk
no flags Details
Make QtGraphicsPluginWidget AC code path friendly (3.22 KB, patch)
2011-04-11 14:26 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Make QtGraphicsPluginWidget AC code path friendly (2) (9.38 KB, patch)
2011-04-27 12:09 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Make QtGraphicsPluginWidget AC code path friendly (take 3) (9.71 KB, patch)
2011-04-27 13:14 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Patch against ToT (9.74 KB, patch)
2011-04-27 13:29 PDT, Girish Ramakrishnan
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bernhard Rosenkraenzer 2010-02-01 03:13:21 PST
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.
Comment 1 Benjamin Poulain 2010-03-05 04:43:53 PST
Could you post an example/reduction for the issue?
Are you sure you use windowless plugins?
Comment 2 Tor Arne Vestbø 2010-03-05 09:39:43 PST
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
Comment 3 Kent Hansen 2010-03-10 06:50:42 PST
Closing until we receive a testcase that demonstrates the issue.
Comment 4 Frank Hanamy 2010-03-11 15:16:55 PST
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
Comment 5 Kent Hansen 2010-03-12 02:36:15 PST
Reopening thanks to Frank's testcase.
Comment 6 Robert Hogan 2010-06-20 08:29:58 PDT
*** Bug 32174 has been marked as a duplicate of this bug. ***
Comment 7 thomas.senyk 2011-02-16 06:18:15 PST
Created attachment 82623 [details]
another example

Another example showing the ignored z-index problem.
Comment 8 thomas.senyk 2011-02-16 06:23:36 PST
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.
Comment 9 Robert Hogan 2011-02-16 14:11:15 PST
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
Comment 10 Robert Hogan 2011-02-16 14:13:53 PST
Created attachment 82692 [details]
My result against trunk

Video plugin obeys z-ordering
Comment 11 thomas.senyk 2011-02-17 05:25:40 PST
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)
Comment 12 Robert Hogan 2011-02-18 10:49:55 PST
(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.
Comment 13 thomas.senyk 2011-03-30 03:15:25 PDT
Created attachment 87493 [details]
another example - updated

Errors fixed in the nokia.html
Comment 14 thomas.senyk 2011-03-30 03:29:29 PDT
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.
Comment 15 thomas.senyk 2011-03-30 07:48:58 PDT
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.
Comment 16 Girish Ramakrishnan 2011-04-11 14:26:57 PDT
Created attachment 89088 [details]
Make QtGraphicsPluginWidget AC code path friendly
Comment 17 Kenneth Rohde Christiansen 2011-04-11 14:31:55 PDT
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
Comment 18 Girish Ramakrishnan 2011-04-11 14:49:15 PDT
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.
Comment 19 Girish Ramakrishnan 2011-04-11 15:07:11 PDT
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 :-)
Comment 20 Girish Ramakrishnan 2011-04-11 15:24:08 PDT
(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).
Comment 21 Kenneth Rohde Christiansen 2011-04-12 01:04:47 PDT
We have autotests already, can't it be added there?
Comment 22 Girish Ramakrishnan 2011-04-19 08:19:36 PDT
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.
Comment 23 Girish Ramakrishnan 2011-04-27 09:59:23 PDT
Customer has confirmed it works. I am writing a test case now so we can try to commit it.
Comment 24 Girish Ramakrishnan 2011-04-27 12:09:19 PDT
Created attachment 91318 [details]
Make QtGraphicsPluginWidget AC code path friendly (2)
Comment 25 Girish Ramakrishnan 2011-04-27 13:14:19 PDT
Created attachment 91331 [details]
Make QtGraphicsPluginWidget AC code path friendly (take 3)

create patch with --binary
Comment 26 Girish Ramakrishnan 2011-04-27 13:29:34 PDT
Created attachment 91333 [details]
Patch against ToT
Comment 27 WebKit Review Bot 2011-04-27 13:32:34 PDT
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.
Comment 28 Girish Ramakrishnan 2011-04-28 02:24:37 PDT
This patch doesn't work with trunk anymore. 

Last known good revision is 83789. It broke somewhere between that and 83914.
Comment 29 Eric Seidel (no email) 2011-05-01 10:56:00 PDT
Comment on attachment 91333 [details]
Patch against ToT

r- since the patch supposedly doesn't work anymore.
Comment 30 Viatcheslav Ostapenko 2011-05-24 08:03:15 PDT
(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.
Comment 31 Girish Ramakrishnan 2011-05-25 09:55:14 PDT
(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.
Comment 32 Simon Hausmann 2011-06-01 05:51:21 PDT
(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 :)
Comment 33 Viatcheslav Ostapenko 2011-06-01 07:24:03 PDT
(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.
Comment 34 Eric Seidel (no email) 2011-06-18 13:31:04 PDT
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.
Comment 35 Gauvain Pocentek 2013-05-04 07:04:00 PDT
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.
Comment 36 Jocelyn Turcotte 2014-02-03 03:16:07 PST
=== 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.