Double-click in div (cite prefix) or blockquote still opens the Advanced property editor, follow-up from bug 1143744

RESOLVED FIXED in Thunderbird 50.0

Status

MailNews Core
Composition
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2), NeedInfo)

Tracking

Trunk
Thunderbird 50.0

Thunderbird Tracking Flags

(thunderbird47 wontfix, thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr45- unaffected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 months ago
Created attachment 8774073 [details] [diff] [review]
Proposed solution (v1a).

Looks like this wasn't all that optimal:
https://hg.mozilla.org/comm-central/rev/862885de2876

Anyway, let's fix it now. When testing, use a reply and click to the right of text in the cite prefix or blockquote.
Attachment #8774073 - Flags: review?(acelists)

Comment 1

10 months ago
Comment on attachment 8774073 [details] [diff] [review]
Proposed solution (v1a).

Review of attachment 8774073 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +1604,5 @@
>  
>      // Don't fire for body/p. It's common that people try to double-click
>      // to select a word, but the click hits an empty area.
> +    if (element &&
> +        !"body,p,blockquote,div".includes(element.nodeName.toLowerCase()))

I'd feel safer with !["body","p","blockquote","div"].includes(element.nodeName.toLowerCase())) :)
Attachment #8774073 - Flags: review?(acelists) → review+
(Assignee)

Comment 2

10 months ago
Created attachment 8774075 [details] [diff] [review]
Proposed solution (v1b).

Addressed review comment. Also fixed up comment a few lines earlier.
Assignee: nobody → mozilla
Attachment #8774073 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8774075 - Flags: review+
(Assignee)

Comment 3

10 months ago
No rush landing this. Could be a handy patch if someone were looking for something to push ;-)
Keywords: checkin-needed
(Assignee)

Comment 4

10 months ago
https://hg.mozilla.org/comm-central/rev/398d89cd600f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 5

10 months ago
Comment on attachment 8774075 [details] [diff] [review]
Proposed solution (v1b).

[Approval Request Comment]
Regression caused by (bug #): bug 1143744
User impact if declined: Double-click sometimes unexpectedly launches Advanced Property Editor.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Low risk, just added div and blockquote to classes to ignore.
Attachment #8774075 - Flags: approval-comm-esr45?
Attachment #8774075 - Flags: approval-comm-aurora+
(Assignee)

Updated

10 months ago
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
(Assignee)

Updated

10 months ago
Blocks: 1143744
(Assignee)

Comment 6

10 months ago
Created attachment 8774350 [details] [diff] [review]
Fixing "pre" as well.

Grr, just tested that on a reply to a BMO mail and hit a pre.
Attachment #8774350 - Flags: review?(acelists)
(Assignee)

Updated

10 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 7

10 months ago
Comment on attachment 8774350 [details] [diff] [review]
Fixing "pre" as well.

Isn't the list getting infinite? Or is there reason to believe there is only a fixed set of tags we need to check here?
(Assignee)

Comment 8

10 months ago
Block elements ;-)

Comment 9

10 months ago
I think anything can be block element if you set it in css. Maybe there is a method to ask Gecko if that element is block?
(Assignee)

Comment 10

10 months ago
(In reply to :aceman from comment #7)
> Isn't the list getting infinite?
In general all resources are finite, and there is only a limited number of HTML tags representing blocks:
https://developer.mozilla.org/en/docs/Web/HTML/Block-level_elements

Yes, there are more than just the ones we've covered: p, div, blockquote and now pre, but for some of them we do want the double-click to act, like for ol, ul, table, hr.

So I suggest to add the obvious omission of pre to the list and be done with it, in fact, while we're at it, add h1-h6 as well ;-)
(Assignee)

Comment 11

10 months ago
Created attachment 8774413 [details] [diff] [review]
Fixing pre and h1-h6 as well

Here we go, that should be complete now.
Attachment #8774350 - Attachment is obsolete: true
Attachment #8774350 - Flags: review?(acelists)

Comment 12

10 months ago
Comment on attachment 8774413 [details] [diff] [review]
Fixing pre and h1-h6 as well

Review of attachment 8774413 [details] [diff] [review]:
-----------------------------------------------------------------

Let's hope so.
Attachment #8774413 - Flags: review+
(Assignee)

Comment 13

10 months ago
Indeed, let's hope so ;-)

Landed second patch:
https://hg.mozilla.org/comm-central/rev/c229bf0c849e
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago10 months ago
Resolution: --- → FIXED
(Assignee)

Comment 14

10 months ago
TB 49 (Aurora):
https://hg.mozilla.org/releases/comm-aurora/rev/2c3312bd6c31
https://hg.mozilla.org/releases/comm-aurora/rev/e3dce3ebf4dd
(landed with DONTBUILD since it's a trivial few-lines change).
status-thunderbird49: affected → fixed
(Assignee)

Updated

10 months ago
Attachment #8774413 - Flags: approval-comm-esr45?
Attachment #8774413 - Flags: approval-comm-aurora+
(Assignee)

Comment 15

10 months ago
Neil, if you didn't like what I did in bug 1143744, you won't like this either:
https://dxr.mozilla.org/comm-central/rev/6949ba7846d2dd02e95ec58624ebf77d7ba691a2/editor/ui/composer/content/editor.js#1609
Can you submit a better solution in a new bug, please.
Flags: needinfo?(neil)
(Assignee)

Updated

8 months ago
Attachment #8774075 - Flags: approval-comm-esr45?
(Assignee)

Updated

8 months ago
Attachment #8774413 - Flags: approval-comm-esr45?

Comment 16

7 months ago
This bug fixes issues introduced in bug 1143744, but since that bug has approval-comm-esr45-, presumably this patch is not needed for comm-esr45.

Updated

4 months ago
status-thunderbird_esr45: affected → unaffected
tracking-thunderbird_esr45: ? → -
You need to log in before you can comment on or make changes to this bug.