Bug 112554 - Tests added in r141354 erroneously assume that spelling markers are not erased after a selection change
Summary: Tests added in r141354 erroneously assume that spelling markers are not erase...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on:
Blocks: 108528
  Show dependency treegraph
 
Reported: 2013-03-18 03:53 PDT by Grzegorz Czajkowski
Modified: 2013-04-30 02:56 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch for disappearing spelling marker (1.71 KB, patch)
2013-04-30 02:46 PDT, Artur Moryc
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2013-03-18 03:53:30 PDT
There's 'shouldEraseMarkersAfterChangeSelection' setting which is exposed to port implementation whether we should erase markers after selection change or not. By default this setting is set on true.

Chromium and Mac (at least for wk2) prefer not erasing markers so they return false for this setting.

A newly added tests (checked for spelling-double-clicked-word.html) assume that selection (as a result of double click on misspelled word) doesn't erase spelling markers. Here is the code:

spellingMarkerRange = internals.markerRangeForNode(textNode, "spelling", 0);    <--- OK, marker exists
....
// Double-click the misspelled word
eventSender.mouseDown();
eventSender.mouseUp();
eventSender.mouseDown();
eventSender.mouseUp();

shouldBeEqualToString("window.getSelection().toString()", "wellcome");          <--- Ok, verifies selection after double click

spellingMarkerRange = internals.markerRangeForNode(textNode, "spelling", 0);    <--- spellingMarkerRange is NULL for platforms whose erase markers after selection change.

I'd like to ask you what's the best way of fixing this issue for other Platforms?

It seems reasonably to check whether ports erase markers or not (we could just check spellingMarkerRange object). As a result we will have to deliver new baseline for other ports (at lest EFL).

Of course we could implement this behaviour similarly to Chromium and Mac but I am not familiar with it. I couldn't see any visual changes while editing misspelled word in MiniBrowser.

Thanks.
Comment 1 Ryosuke Niwa 2013-03-18 08:20:36 PDT
Why don't we set shouldEraseMarkersAfterChangeSelection to false in the test then?
Comment 2 Rouslan Solomakhin 2013-03-18 09:29:33 PDT
I think that this tests is checking for more things that necessary. I think that you can safely remove the failing assumption from the test and the expectation file.
Comment 3 Rouslan Solomakhin 2013-03-18 09:30:09 PDT
(In reply to comment #1)
> Why don't we set shouldEraseMarkersAfterChangeSelection to false in the test then?

This works, too.
Comment 4 Ryosuke Niwa 2013-04-01 21:02:55 PDT
Also see 113742.
Comment 5 Artur Moryc 2013-04-24 08:37:40 PDT
>I couldn't see any visual changes while editing misspelled word in MiniBrowser.

What I have noticed so far in MiniBrowser with respect to the returned value of shouldEraseMarkersAfterChangeSelection is:

1) true 

the marker for the misspelled word disappears while the mouse cursor is being navigated along the misspelled word


2) false 
 
the misspelled word remains underlined regardless the mouse cursor position
Comment 6 Grzegorz Czajkowski 2013-04-24 23:56:22 PDT
(In reply to comment #5)
> >I couldn't see any visual changes while editing misspelled word in MiniBrowser.
> 
> What I have noticed so far in MiniBrowser with respect to the returned value of shouldEraseMarkersAfterChangeSelection is:
> 
> 1) true 
> 
> the marker for the misspelled word disappears while the mouse cursor is being navigated along the misspelled word
> 
> 
> 2) false 
> 
> the misspelled word remains underlined regardless the mouse cursor position

Thanks Artur for checking the setting.
IMO it seems reasonable to implement 'shouldEraseMarkersAfterChangeSelection' for EFL similarly to Mac (no to erase spelling markers after selection/cursor change).

  (In reply to comment #2)
> I think that this tests is checking for more things that necessary. I think that you can safely remove the failing assumption from the test and the expectation file.

Agree. It will be helpful for ports that prefer default behaviour.
Comment 7 Artur Moryc 2013-04-30 02:46:48 PDT
Created attachment 200094 [details]
Proposed patch for disappearing spelling marker
Comment 8 Artur Moryc 2013-04-30 02:56:31 PDT
Comment on attachment 200094 [details]
Proposed patch for disappearing spelling marker 

This patch should go to 115165