Summary: | [Qt] QWebPluginFactory based plugins are always painted on top, ignore z-index | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bernhard Rosenkraenzer <bero> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Girish Ramakrishnan <girish> | ||||||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||||||
Severity: | Normal | CC: | benedikte.holm, benjamin, gauvain, girish, hausmann, kenneth, kent.hansen, ostap73, robert, thomas.senyk, webkit.review.bot | ||||||||||||||||||||
Priority: | P3 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 57418 | ||||||||||||||||||||||
Attachments: |
|
Description
Bernhard Rosenkraenzer
2010-02-01 03:13:21 PST
Could you post an example/reduction for the issue? Are you sure you use windowless plugins? 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 Closing until we receive a testcase that demonstrates the issue. 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
Reopening thanks to Frank's testcase. *** 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. 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 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) (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.
Created attachment 89088 [details]
Make QtGraphicsPluginWidget AC code path friendly
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 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. 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 :-) (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). We have autotests already, can't it be added there? 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. Customer has confirmed it works. I am writing a test case now so we can try to commit it. Created attachment 91318 [details]
Make QtGraphicsPluginWidget AC code path friendly (2)
Created attachment 91331 [details]
Make QtGraphicsPluginWidget AC code path friendly (take 3)
create patch with --binary
Created attachment 91333 [details]
Patch against ToT
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.
This patch doesn't work with trunk anymore. Last known good revision is 83789. It broke somewhere between that and 83914. Comment on attachment 91333 [details]
Patch against ToT
r- since the patch supposedly doesn't work anymore.
(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. (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. (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 :) (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 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. 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. === 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. |