Make WordPress Core

Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#15911 closed defect (bug) (fixed)

Comment Quick Edit quirks

Reported by: garyc40's profile garyc40 Owned by: azaozz's profile azaozz
Milestone: 3.3 Priority: normal
Severity: minor Version: 3.0
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

Two problems exist from 3.0.3:

  1. Wrong textarea height when editing a long comment (see screenshot)
  1. Quicktags are not properly closed when canceling a quick edit form, or navigating to another page

Attachments (6)

quickedit-height.png (42.2 KB) - added by garyc40 14 years ago.
garyc40-15911.patch (1.1 KB) - added by garyc40 14 years ago.
there's a patch for that
15911.2.patch (1.0 KB) - added by SergeyBiryukov 13 years ago.
15911.3.patch (1.1 KB) - added by SergeyBiryukov 13 years ago.
15911.4.patch (1.3 KB) - added by SergeyBiryukov 13 years ago.
15911.5.patch (1.2 KB) - added by SergeyBiryukov 13 years ago.

Download all attachments as: .zip

Change History (20)

#1 @nacin
14 years ago

  • Component changed from General to Comments
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 3.0

@garyc40
14 years ago

there's a patch for that

#2 @garyc40
14 years ago

  • Keywords has-patch needs-testing added

#3 @ocean90
14 years ago

  • Keywords needs-refresh added; 3.2-early needs-testing removed
  • Milestone changed from Future Release to 3.3

Point 1 can be fixed with #18348.

#4 @SergeyBiryukov
13 years ago

  • Keywords needs-refresh removed

Related: #16695

Refreshed the patch for point 2.

Since replycontent becomes empty after cancelling Quick Edit form, closeAllTags() should just clear button states (unlike CloseButton(), which actually changes content).

#5 @SergeyBiryukov
13 years ago

15911.3.patch gives the function a more appropriate title.

15911.4.patch fixes and keeps the old function as well. Currently it's not used anywhere though, so I prefer 15911.3.patch.

#6 @azaozz
13 years ago

Not 100% sure we need this but it is nice to stop "polluting" the textarea when it's empty and we still have open tags. Think we can add this directly in qt.TagButton.prototype.callback(), will make a patch soon.

#7 @azaozz
13 years ago

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

In [18576]:

Fix QTags.closeAllTags(), replace 'tb' with 'ed' in quicktags,js to make it clear it is the editor instance not the toolbar, small comments quick edit fixes, see #16695, fixes #15911

#8 @SergeyBiryukov
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

When cancelling Quick Edit form with some open tags in it, calling closeAllTags() from commentReply.close() floods Firefox JS console (6.0/Windows, tried without extensions too) with errors:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLTextAreaElement.selectionStart]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://trunk.wordpress/wp-includes/js/quicktags.dev.js?ver=20110820 :: <TOP_LEVEL> :: line 410"  data: no]

And the comment disappears until page reload.

If it's called earlier (e.g. before fadeOut() in commentReply.revert()), everything works as expected, though we can briefly see the tags closing, and these changes will be discarded anyway.

That's why I thought about just clearing button states, without touching replycontent if it's invisible.

#9 follow-up: @SergeyBiryukov
13 years ago

And the comment disappears until page reload.

Same in IE 8.

15911.5.patch introduces clearButtonStates(), which fixes the issue. If it should be fixed in some other way, I'd like to know.

#10 @jane
13 years ago

  • Owner garyc40 deleted
  • Status changed from reopened to assigned

Removing garyc as owner so a committer will look at ticket (when it is already "owned" by someone it gets ignored by the others).

#11 @azaozz
13 years ago

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

In [19290]:

Prevent errors in QTags.closeAllTags(), make it remove button states but not modify the content (as that often is wrong anyways), fixes #15911

#12 in reply to: ↑ 9 ; follow-up: @azaozz
13 years ago

Replying to SergeyBiryukov:

Imho we shouldn't "close all tags" automatically. That often produces broken HTML. Not sure what's worse: leaving a tag open because the user forgot to close it or inserting a close tag in the wrong place.

#13 in reply to: ↑ 12 ; follow-up: @SergeyBiryukov
13 years ago

Replying to azaozz:

Imho we shouldn't "close all tags" automatically.

Agreed. I guess I wasn't suggesting that :) Was trying to find a proper way to clear button states.

#14 in reply to: ↑ 13 @azaozz
13 years ago

Replying to SergeyBiryukov:

Was trying to find a proper way to clear button states.

Yes, and I almost committed the patch (works well btw). But then ran some tests on the edit post screen and the HTML was "more" broken if we force-closed all tags at the end.

Note: See TracTickets for help on using tickets.