Bug 60311 – [Qt] fast/events/backspace-nagivates-back fails on Qt bots (Mac and Linux)

• Component: UI Events
• Status: RESOLVED FIXED

[Evan Martin]

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?

[Antonio Gomes]

> 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

[Evan Martin]

Created attachment 92525 [details]
Patch

[Evan Martin]

I landed this in the skipped list in r85910.

http://trac.webkit.org/changeset/85910

[Antonio Gomes]

Evan, it would probably be better to skip it for all qt platforms

platform/qt-linux/Skipped is only for Qt on linux. Ossy?

[Csaba Osztrogonác]

(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

[Antonio Gomes]

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

[Evan Martin]

Ah, setEditingBehavior isn't implemented!  I am glad there is not a more fundamental reason the test doesn't work.

[Antonio Gomes]

(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");

[Ojan Vafai]

(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?

[Antonio Gomes]

(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?)

[Evan Martin]

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?

[Csaba Osztrogonác]

(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

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 93588 [details]
Patch

[Evan Martin]

Antonio, does this look correct to you?

[WebKit Commit Bot]

Comment on attachment 93588 [details]
Patch

Clearing flags on attachment: 93588

Committed r87611: <http://trac.webkit.org/changeset/87611>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.