Closed Bug 1288929 Opened 4 years ago Closed 4 years ago

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


(MailNews Core :: Composition, defect)

Not set


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

Thunderbird 50.0
Tracking Status
thunderbird47 --- wontfix
thunderbird48 --- wontfix
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr45 - unaffected


(Reporter: jorgk-bmo, Assigned: jorgk-bmo)




(2 files, 2 obsolete files)

Attached patch Proposed solution (v1a). (obsolete) — Splinter Review
Looks like this wasn't all that optimal:

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 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+
Addressed review comment. Also fixed up comment a few lines earlier.
Assignee: nobody → mozilla
Attachment #8774073 - Attachment is obsolete: true
Attachment #8774075 - Flags: review+
No rush landing this. Could be a handy patch if someone were looking for something to push ;-)
Keywords: checkin-needed
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
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+
Blocks: 1143744
Attached patch Fixing "pre" as well. (obsolete) — Splinter Review
Grr, just tested that on a reply to a BMO mail and hit a pre.
Attachment #8774350 - Flags: review?(acelists)
Resolution: FIXED → ---
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?
Block elements ;-)
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?
(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:

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 ;-)
Here we go, that should be complete now.
Attachment #8774350 - Attachment is obsolete: true
Attachment #8774350 - Flags: review?(acelists)
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+
Indeed, let's hope so ;-)

Landed second patch:
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment #8774413 - Flags: approval-comm-esr45?
Attachment #8774413 - Flags: approval-comm-aurora+
Neil, if you didn't like what I did in bug 1143744, you won't like this either:
Can you submit a better solution in a new bug, please.
Flags: needinfo?(neil)
Attachment #8774075 - Flags: approval-comm-esr45?
Attachment #8774413 - Flags: approval-comm-esr45?
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.
Flags: needinfo?(neil)
You need to log in before you can comment on or make changes to this bug.