Closed Bug 1143744 Opened 5 years ago Closed 4 years ago

Table properties don't open by double-click of table cell. Image properties don't open by double-click on right part of image.

Categories

(MailNews Core :: Composition, defect)

defect
Not set

Tracking

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

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

People

(Reporter: a0005408, Assigned: jorgk-bmo)

References

Details

(Keywords: regression, Whiteboard: [regression:TB31])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

Thunderbird v. 31.5.0
Opened a HTML- composition window.
Insert a table (insert > table..) > OK.
Doubleclicked into an empty field of the table.



Actual results:

No reaction.


Expected results:

It should be opened the "table properties". 
This happened still in v 24.1 (on my old portable version).
An other user maintaines that it doesn't work only since v. 31.3.

You can reach the properties menu problem-freeclicking on a table cell via > format > table > table properties.
Jorgk, any idea if this was an intentional change?
Component: Untriaged → Composition
Flags: needinfo?(mozilla)
Product: Thunderbird → MailNews Core
Summary: Table properties don't open by doubleclick → Table properties don't open by doubleclick of table cell
Version: 31 Branch → 31
Wayne, thanks for alerting us to this bug ;-)

This is a regression from
https://hg.mozilla.org/comm-central/rev/107302e430f1
It says that it's bug 525125, but that's clearly wrong. I found the bug by searching for "107302e430f1" in a comment: Bug 525124 which landed on TB 31.
I backed it out locally and the table properties come up again when double-clicking behind the content of the cell.

Also, the image properties come up reliably when backing this out, see bug 1246094 which has caused us a lot of headache.

I think that change smashed a lot of things :-(
I don't know what is the lesser evil: The problem of bug 525124 that the Advanced Property Editor is coming up or smashing all other special double-click actions. Grrrrr.

What Neil did there is really bad. Aceman, how can we fix this? Two experts reviewed this so three people failed here.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mozilla) → needinfo?(acelists)
Keywords: regression
I don't understand why they didn't land Magnus' patch back in bug 525124: Attachment 8402391 [details] [diff].

That did exactly what was needed.
I'm out of my element here, but I'm wondering whether double click _should_ open properties
Blocks: 525124
Whiteboard: [regression:TB31]
Magnus, look what Neil & Co. have broken here. Your patch was clear and straight forward and they just ignored it. And now, 19 versions later, we need to go in and fix it again after it caused so many other problems. Sigh.

NI: Just for your information.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(acelists) → needinfo?(mkmelin+mozilla)
Attachment #8762297 - Flags: review?(acelists)
Summary: Table properties don't open by doubleclick of table cell → Table properties don't open by double-click of table cell. Image properties don't open by double-click on right part of image.
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #4)
> I'm out of my element here, but I'm wondering whether double click _should_
> open properties

Yes, it should. And double-clicking an image should also open the image properties. It always did until it got smashed. So let's un-smash it now!
So this is why I couldn't get Table properties to open when debugging the editor dialogs recently? :)

But other dialogs worked, why is only the table element affected? Is it because the table contains the other elements which may also have their own dialogs (that we want access too) ?
It's because table elements are followed by <br>, use ThunderHTMLedit to see it.

I'm taking the liberty to land this now. I am reviewing Magnus' patch and I'm giving it r+ ;-)
Comment on attachment 8762297 [details] [diff] [review]
Backing out Neil's fix from bug 525124 and using Magnus' approach instead.

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

This is Magnus' patch from bug 525124, it is clear and simple and I have the right to review it.
r=jorgk.
Attachment #8762297 - Flags: review?(acelists) → review+
https://hg.mozilla.org/comm-central/rev/862885de2876

As I said: I reviewed the attachment 8402391 [details] [diff] [review] by Magnus from bug 525124 and found it good.
I am also backing out Neil's faulty fix which has caused this regression which initially started with table properties not opening, but in fact also affected image properties.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(mkmelin+mozilla)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
Comment on attachment 8762297 [details] [diff] [review]
Backing out Neil's fix from bug 525124 and using Magnus' approach instead.

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

You can get my support :)

This allows me to open table properties again. Also an image inside table works.
Also it seems you can click anywhere in the image to get its properties (not only in the first half of it, as discussed in bug 1246094).

But I'll leave a request to Neil here, maybe he can find out why this solution didn't work for him (or Seamonkey).
Attachment #8762297 - Flags: review+
Attachment #8762297 - Flags: feedback?(neil)
OS: Windows 7 → All
Hardware: x86_64 → All
Thanks. Now six eyes have looked at it ;-)
Comment on attachment 8762297 [details] [diff] [review]
Backing out Neil's fix from bug 525124 and using Magnus' approach instead.

[Approval Request Comment]
Regression caused by (bug #): Bug 525124.
User impact if declined: Table and Image properties don't open on double-click.
Testing completed (on c-c, etc.): Manual.
Double-click on image is also tested by test-image-insertion-dialog.js.
Risk to taking this patch (and alternatives if risky):
Low risk. Backed out faulty fix and landed very local fix instead.
Attachment #8762297 - Flags: approval-comm-esr45?
Attachment #8762297 - Flags: approval-comm-beta+
Attachment #8762297 - Flags: approval-comm-aurora+
Actually "The problem of bug 525124 that the Advanced Property Editor" was not a problem for me at all. but instead, a convenient way to open the advanced editor. (now it's much more difficult)
Wondering if that behavior is restored (will test when I update trunk.
No, it's now restored. I couldn't regress the other bug. Now if you double-click in "free space" nothing will happen. It's just implemented some other way.
Typo: No, it's NOT restored.
Kent, this wasn't quite right, so if you land this on ESR45, please include a further fix from bug 1288929.
Comment on attachment 8762297 [details] [diff] [review]
Backing out Neil's fix from bug 525124 and using Magnus' approach instead.

I'm not convinced that Magnus's solution is ideal, but then my editor glasses have a different colour to his Thunderbird glasses, and you don't worry about All Tags mode for instance, which you might have broken for us. Also, I'm not sure what behaviour you get if you insert a heading inside a table cell.

Anyway, I agree that rangeParent isn't what we want here, because it isn't what I thought it was at the time. Perhaps what we should be doing is to call GetObjectForProperties()? (That's the function used to populate the Format - Advanced Properties menuitem.)
Attachment #8762297 - Flags: feedback?(neil)
Hmmm, the first patch was "low risk" but somehow risky enough to introduce a regression that had to be fixed in a followup bug. There is also a clear alternative, right? (Format/Table/Table Properties...)

I'm not seeing any compelling reason to take this in 45.4.0
Attachment #8762297 - Flags: approval-comm-esr45? → approval-comm-esr45-
You need to log in before you can comment on or make changes to this bug.