Bug 60311 - [Qt] fast/events/backspace-nagivates-back fails on Qt bots (Mac and Linux)
Summary: [Qt] fast/events/backspace-nagivates-back fails on Qt bots (Mac and Linux)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Unspecified
: P2 Normal
Assignee: Evan Martin
URL:
Keywords:
: 60349 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-05-05 16:20 PDT by Evan Martin
Modified: 2011-05-28 05:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.12 KB, patch)
2011-05-05 18:42 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (1.23 KB, patch)
2011-05-15 11:45 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Martin 2011-05-05 16:20:16 PDT
I just checked in a test that tries to verify that backspace navigates back.
The test tests the per-platform behavior for all platforms known by EditingBehavior (mac, win, unix).

It fails on Qt.  Test output:
This test passes if it says PASS below.
FAIL: expected navigation to test-complete

http://build.webkit.org/results/Qt%20Linux%20Release/r85893%20(32469)/results.html

This indicates that somehow pressing "back" *is* causing a navigation.  I'm not sure how to diagnose it.
Can you help?  Should I skip the test to green the tree?
Comment 1 Antonio Gomes 2011-05-05 17:22:22 PDT
> This indicates that somehow pressing "back" *is* causing a navigation.  I'm not sure how to diagnose it.
> Can you help?  Should I skip the test to green the tree?

I can have a look. Please skip it, Evan. Ty
Comment 2 Evan Martin 2011-05-05 18:42:11 PDT
Created attachment 92525 [details]
Patch
Comment 3 Evan Martin 2011-05-05 18:43:42 PDT
I landed this in the skipped list in r85910.

http://trac.webkit.org/changeset/85910
Comment 4 Antonio Gomes 2011-05-06 07:15:13 PDT
Evan, it would probably be better to skip it for all qt platforms

platform/qt-linux/Skipped is only for Qt on linux. Ossy?
Comment 5 Csaba Osztrogonác 2011-05-06 07:16:37 PDT
(In reply to comment #4)
> Evan, it would probably be better to skip it for all qt platforms
> 
> platform/qt-linux/Skipped is only for Qt on linux. Ossy?
yes, but we use platform/qt/Skipped because historically reason
Comment 6 Antonio Gomes 2011-05-06 07:47:14 PDT
*** Bug 60349 has been marked as a duplicate of this bug. ***
Comment 7 Evan Martin 2011-05-06 08:39:16 PDT
Ah, setEditingBehavior isn't implemented!  I am glad there is not a more fundamental reason the test doesn't work.
Comment 8 Antonio Gomes 2011-05-06 08:44:56 PDT
(In reply to comment #7)
> Ah, setEditingBehavior isn't implemented!  I am glad there is not a more fundamental reason the test doesn't work.

I implemented that myself :)

> grep -nHR setEditingBehavior .
./LayoutTestControllerQt.cpp:771:void LayoutTestController::setEditingBehavior(const QString& editingBehavior)
./LayoutTestControllerQt.cpp:773:    DumpRenderTreeSupportQt::setEditingBehavior(m_drt->webPage(), editingBehavior);
./LayoutTestControllerQt.h:251:    void setEditingBehavior(const QString& editingBehavior);
./DumpRenderTreeQt.cpp:586:    DumpRenderTreeSupportQt::setEditingBehavior(m_page, "win");
Comment 9 Ojan Vafai 2011-05-06 09:10:50 PDT
(In reply to comment #4)
> Evan, it would probably be better to skip it for all qt platforms
> 
> platform/qt-linux/Skipped is only for Qt on linux. Ossy?

When we looked at the Qt bots, it was only failing on the Qt-linux bots. Did we misread them?
Comment 10 Antonio Gomes 2011-05-06 09:23:31 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Ah, setEditingBehavior isn't implemented!  I am glad there is not a more fundamental reason the test doesn't work.
> 
> I implemented that myself :)
> 
> > grep -nHR setEditingBehavior .
> ./LayoutTestControllerQt.cpp:771:void LayoutTestController::setEditingBehavior(const QString& editingBehavior)
> ./LayoutTestControllerQt.cpp:773:    DumpRenderTreeSupportQt::setEditingBehavior(m_drt->webPage(), editingBehavior);
> ./LayoutTestControllerQt.h:251:    void setEditingBehavior(const QString& editingBehavior);
> ./DumpRenderTreeQt.cpp:586:    DumpRenderTreeSupportQt::setEditingBehavior(m_page, "win");

Evan, did I miss something?

(In reply to comment #9)
> (In reply to comment #4)
> > Evan, it would probably be better to skip it for all qt platforms
> > 
> > platform/qt-linux/Skipped is only for Qt on linux. Ossy?
> 
> When we looked at the Qt bots, it was only failing on the Qt-linux bots. Did we misread them?

The Qt-Mac bot is probably not a core builder nor is in webkit waterfall. see it here: http://build.webkit.sed.hu/waterfall (ossy could you correct me?)
Comment 11 Evan Martin 2011-05-06 09:46:40 PDT
Oh, my comment was based on this commit message:
"* platform/mac-wk2/Skipped: Add fast/events/backspace-navigates-back.html because of missing layoutTestController.setEditingBehavior()."

But that must be wk2 specific.

Maybe this does fail for some more fundamental issue.  :(
Does it fail the same way on every bot?
Comment 12 Csaba Osztrogonác 2011-05-10 05:35:00 PDT
(In reply to comment #11)
> Oh, my comment was based on this commit message:
> "* platform/mac-wk2/Skipped: Add fast/events/backspace-navigates-back.html because of missing layoutTestController.setEditingBehavior()."
> 
> But that must be wk2 specific.
WK2 has its own layoutTestController implementation which doesn't have setEditingBehavior() now.
 
> Maybe this does fail for some more fundamental issue.  :(
> Does it fail the same way on every bot?
Yes, Qt Linux, Qt ARM and Qt Mac fails are same.

I updated the Skipped lists: http://trac.webkit.org/changeset/86143
Comment 13 Robert Hogan 2011-05-15 11:36:09 PDT
Hi Evan,

This is happening because EditorClientQt::handleKeyboardEvent() will only honour the unix editing behaviour (by interpreting the backspace as an edit command) if the selected text is editable. Since this isn't the case here, it ends up in QWebPagePrivate::keyPressEvent(QKeyEvent *ev) which interprets the command as a navigation command. Whereas your test treats it as a no-op, am I correct?

I think this just requires a platform-specific result for Qt, since it will always treat a backspace as a page->back() if there is no text to edit - regardless of platform.
Comment 14 Robert Hogan 2011-05-15 11:45:02 PDT
Created attachment 93588 [details]
Patch
Comment 15 Evan Martin 2011-05-25 12:39:08 PDT
Antonio, does this look correct to you?
Comment 16 WebKit Commit Bot 2011-05-28 05:15:40 PDT
Comment on attachment 93588 [details]
Patch

Clearing flags on attachment: 93588

Committed r87611: <http://trac.webkit.org/changeset/87611>
Comment 17 WebKit Commit Bot 2011-05-28 05:15:46 PDT
All reviewed patches have been landed.  Closing bug.