Opened 7 years ago

Closed 7 years ago

#5450 closed Bug (fixed)

Press enter on 'replace' button result wrong

Reported by: garry.yao Owned by: brooks
Priority: Normal Milestone: CKEditor 3.3
Component: UI : Dialogs Version:
Keywords: Review+ Cc:

Description

Reproducing Procedures

  1. Load the any of the sample page;
  2. Open 'Replace' dialog and fill in 'are';
  3. Tab to focus the 'Replace All' button and press 'Enter';
  • Expected Result: You get only the success message.
  • Actual Result: You get both message: 'Specified text not found' and '1 occurance replaced'.

Attachments (3)

5450.patch (947 bytes) - added by brooks 7 years ago.
5450_2.patch (787 bytes) - added by brooks 7 years ago.
5450_3.patch (532 bytes) - added by brooks 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by brooks

  • Owner set to brooks
  • Status changed from new to assigned

Changed 7 years ago by brooks

comment:2 Changed 7 years ago by brooks

  • Keywords Review? added; Confirmed removed

this bug only happen in firefox, it's a weired behavior

when debugger, e.g, add break point in source code, u can't reproduce the bug

I trace to here:
_source/plugins/dialogui/plugin.js#503

element.on( 'click', function( evt )
{   console.log( 'click fired!' );
	me.fire( 'click', { dialog : me.getDialog() } );
	evt.data.preventDefault();
} );

element.on( 'keydown', function( evt )
{
	if ( evt.data.getKeystroke() in { 32:1, 13:1 } )
	{	console.log( 'keydown fired!' );
		me.click();
		evt.data.preventDefault();
	}
} );

then found when hit enter,the click event fired too.

comment:3 Changed 7 years ago by garry.yao

  • Keywords Review- added; Review? removed

this bug only happen in firefox, it's a weired behavior

It's indeed a Firefox bug, I believe the default event behavior is somehow affected by the focus (alert box grab the focus from button). While the real problem is that the [http://dev.fckeditor.net/browser/CKEditor/trunk/_source/plugins/dialogui/plugin.js#L507

click simulating logics] are not needed at all, since most major browsers and assistive technologies trigger onClick if the Enter key is pressed when the link has focus.

Changed 7 years ago by brooks

comment:4 Changed 7 years ago by brooks

  • Keywords Review? added; Review- removed

yes, in firefox, when a link is focused and then hit enter key, it will fire click event and couldn't be prevent.

comment:5 Changed 7 years ago by garry.yao

  • Keywords Review- added; Review? removed

It's not a FF only behavior, actual all browsers agree with it on link, and our buttons are all made of link, so IMO we just need to remove the enterkey delegation in all places:

if ( evt.data.getKeystroke() in { 32:1 } )

Changed 7 years ago by brooks

comment:6 Changed 7 years ago by brooks

  • Keywords Review? added; Review- removed

the FF only behavior is that when a link is focused,the enterkey delegation couldn't be prevented,but other browsers could.

of course,it's a nice idea not to listen the keydown event for enter key in this case, 'cause the click event will always fired when hit a focused link.

comment:7 Changed 7 years ago by garry.yao

  • Keywords Review+ added; Review? removed

Please remove the extra spaces on committing.

comment:8 Changed 7 years ago by brooks

  • Resolution set to fixed
  • Status changed from assigned to closed

fixed with 5402 5402

Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy