1486051 - Fix TB/Lightning after landing of bug 1457216: Copy support for editable menulists to C-C

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1457216 will remove the menulist editable="true" binding. If it would only be the binding/CSS we could restore it in c-c but https://hg.mozilla.org/try/rev/aeb4763db7519e88e562693d83016d8e1121df2d shows that also cpp changes are planned.

We use this menulist type in calendar, editor and TB's emailWizard.xul.

How could we workaround this removal? Could we insert through JS maybe a textbox?

Comment 1

[Jorg K (CEST = GMT+2)]

Maybe Magnus is also interested since he's looking into de-XBL in general.

Comment 2

[Tim Nguyen :ntim]

Sounds like textbox + autocomplete dropdown would be a decent replacement (something similar to Firefox's awesome bar) implemented as a binding.

Comment 3

[Magnus Melin [:mkmelin]]

The usage in comm-central is quite limited.

emailWizard.xul: like :ntim suggests in comment 2. Alternatively this can be open coded as the usage is so small.

EdAdvancedEdit.xul: it's hard to even end up in the dialog. Even harder to trigger color suggestions and such which is what the editable menulist is used for... Falls into the category we-have-it-because-we-inherited-editor - there's fairly little reason to have something like this in an email client. 

datetimepickers.xml: I do wonder if the bindings using menulist-editable can't just be tossed now. <input type="date"> and <input type="time" are now avialable so it would be preferable just to use those - they are similar but not identical. In case there are shortcomings, those would be good to fix in platform code.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #3)
> Falls into the category we-have-it-because-we-inherited-editor -
> there's fairly little reason to have something like this in an email client. 
One of TB's strength is that it has a fairly good HTML editor, let's not deconstruct this now. Our primary task is not to win over people who are happy editing e-mail in a web UI with pre-canned/restricted editing features.

Our friends from Postbox have even gone a step further and integrated a full-blown HTML editor into their product, I wonder why: https://www.postbox-inc.com/features/html-editor. I suspect that unlike us, they actually know what their users want and need.

Comment 5

[Jorg K (CEST = GMT+2)]

There appears to be more than what was advertised :-(
https://searchfox.org/comm-central/search?q=menulist.*editable%3D%22true%22&case=false&regexp=true&path=*.x*
(No idea why the link doesn't work, so type |menulist.*editable| into the searchbox and tick Regexp searching on *.x*).

emailWizard.xul doesn't show since it's on different lines.

Geoff, can you please take a look. Bug 1457216 has r+ and will land soon ... and then we're busted again :-(

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1486051-menulists-1.diff

This does the minimum required to un-bust us.

Comment 7

[Geoff Lankow (:darktrojan)]

I've looked at just using <input type="date"> in calendar, the major problem is that the dropdown doesn't work in XUL documents.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004446 [details] [diff] [review]
1486051-menulists-1.diff

Thanks, that's adopting the bindings into C-C for now. What about the C++ changes in this changeset:
https://hg.mozilla.org/try/rev/aeb4763db7519e88e562693d83016d8e1121df2d

Comment 9

[Geoff Lankow (:darktrojan)]

I don't think there's a lot we can do about it.

Comment 10

[Richard Marti (:Paenglab)]

Then we will have accessibility problems.

Comment 11

[Jorg K (CEST = GMT+2)]

See bug 1457216 comment #7. In the medium term we need to find replacements as per comment #3.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Richard Marti (:Paenglab) from comment #10)
> Then we will have accessibility problems.

right, porting the XBL binding to c-c does not address accessibility issue. If XUL autocomplete textbox stays for a while, then it could be a replacement.

Comment 13

[Richard Marti (:Paenglab)]

(In reply to alexander :surkov (:asurkov) from comment #12)
> If XUL autocomplete textbox stays for a while, then it could be a
> replacement.

In accountWizard we present defined ports or servers in the menulist and these can then be modified in the textbox. How could this be done with a autocomplete textbox?

Comment 14

[Jorg K (CEST = GMT+2)]

I'll have to land this now since bug 1457216 has landed.

Comment 15

[Jorg K (CEST = GMT+2)]

I wanted to give this a quick spin before landing, but TB is busted completely, not even a folder pane visible. Errors are:

JavaScript error: chrome://messenger/content/folderPane.js, line 2241: TypeError: this._tree is null, can't access property "invalidateRow" of it
JavaScript error: chrome://messenger/content/folderPane.js, line 2255: TypeError: this._tree is null, can't access property "invalidateRow" of it

Comment 16

[Jorg K (CEST = GMT+2)]

OK, found it. We can test this patch a little before landing after bug 1487035 gets fixed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e92d224e18b9
Port bug 1457216: Copy editable menulist bindings from M-C to C-C. rs=bustage-fix,jorgk

Comment 18

[Jorg K (CEST = GMT+2)]

OK, I looked at some advanced image properties on Windows and the formatting is not very good. Apparently the Linux CSS was moved to common/. So what's the way forward here?

Comment 19

[Richard Marti (:Paenglab)]

I'm on a patch to fix it for Mac and Windows.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e6936052bf60
Follow-up: fix linting problems. r=me DONTBUILD

Comment 21

[Richard Marti (:Paenglab)]

Attachment: menulists-editable.patch

This fixes the appearances on Mac and Windows through splitting the styles to the platforms.

On Windows to before the removal the dropmarker of a hovered editable menulist isn't highlighted.

I had to reorder the stylesheets in emailWizard.xul because it oversteered the rule for .menulist-editable-box in accountCreation.css on Linux and Windows.

Comment 22

[Jorg K (CEST = GMT+2)]

Looks one usage was overlooked, this we have this test failure:
mozmill/composition/test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_editing_identity

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004870 [details] [diff] [review]
menulists-editable.patch

Hmm, the advanced edit panel of an image still doesn't look right. The attribute box is too high. It's already 1px too high on TB 60, but now it's even higher.

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #22)
> Looks one usage was overlooked, this we have this test failure:
> mozmill/composition/test-newmsg-compose-identity.js |
> test-newmsg-compose-identity.js::test_editing_identity
Richard sent me a patch adding
<?xml-stylesheet href="chrome://messenger/content/menulist.css" type="text/css"?>
to messengercompose.xul but
identityElement.editable = true; in MsgComposeCommands.js gives:
MsgComposeCommands.js, line 5856: TypeError: identityElement.select is not a function

Comment 25

[Richard Marti (:Paenglab)]

Attachment: menulist-editable.patch

This fixes the issue with the too high boxes.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004979 [details] [diff] [review]
menulist-editable.patch

OK, this works on Windows, I don't know about the other platforms.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: Bug1486051-fup.patch

Sorry, I was so focused on "editable" that I didn't read the error correctly:
MsgComposeCommands.js, line 5856: TypeError: identityElement.select is not a function

Geoff, can you take another look, please. See comment #22 and comment #24.

This is Richard's patch adding the one line.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/eae99a744e3c
Follow-up: Style the menulist correctly on Mac and Windows. r=jorgk

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: 1486051-identity-editable-1.diff

Not far off, but a regular menulist doesn't have an editable property any more, so setting it to true does nothing. Plus the test waits for the property being true to continue.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 9005117 [details] [diff] [review]
1486051-identity-editable-1.diff

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

Thanks, works for me, I'm glad it wasn't a complicated fix. I'll land it once the build bustage is fixed.

::: common/bindings/menulist.xml
@@ +58,5 @@
>              return val;
>          ]]></body>
>        </method>
>  
> +      <property name="editable" readonly="true" onget="return true;"/>

Care to educate the reviewer? ;-) - It's readonly, yet, we set it? Well, this is the property, however, we set the attribute. Maybe I'll learn XBL before it gets removed ;-(

Comment 31

[Geoff Lankow (:darktrojan)]

Sure. We're not using this binding until we set the attribute. This property only exists if the attribute exists, therefore it's always true. I could've made a setter too, but can't really see a point in it.

Comment 32

[Jorg K (CEST = GMT+2)]

This worked as per my try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=482f382b7736161998b65fbf7d7a429ecdee6157

I'm landing the last patch here now and I'll close the bug. Arshad, can you please file a bug for follow-up work to remove the forked code again and implement it in a different way.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bf51f2112d4f
Fix editable identity menulist in compose window. r=jorgk DONTBUILD

Comment 34

[Arshad Khan [:arshad]]

I have opened a bug for it. I ll try to implement it once I get done with the patches that are already in review.

Comment 35

[Magnus Melin [:mkmelin]]

Bug 1487554. Please always mention bug numbers when you know them.

Comment 36

[Jorg K (CEST = GMT+2)]

And CC the relevant people on the bug when creating it.

Comment 37

[Magnus Melin [:mkmelin]]

Many times for followups it's easiest to just clone the bug (link below) and adjust some fields.

Comment 38

[Arshad Khan [:arshad]]

Noted.