887242 - Add IDs to more of the XUL elements in messengercompose.xul

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Josiah Bruner [:jsbruner]]

The Compose window's layout structure lacks many IDs for it's elements that could be used by extensions/future development. This is a more trivial bug for adding extra IDs here.

Benefit: This will allow extension developers to more easily tamper with the UI as well as making ChromEdit more useful. Future development will be slightly easier.

Cost: Very little. Perhaps will bitrot a few patches, but those can be fixed easily.

I'll have a patch up later today...

Comment 1

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Well, was really busy so the whole: "Later today" thing didn't happen. Anyway, here's a patch that adds a good chunk of IDs to MessengerCompose.xul.

Richard, do you think more menuitems here should have IDs? Did I add too many? Etc.

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 769081 [details] [diff] [review]
Patch.

This looks good. Can you set the IDs as the first item/attribute or whatever this is named, like the other already having an ID (I know it has some which aren't on the first position, but the most are)?

I would say you have more or less gave every element an ID ;). I see only some select_all items, but these don't need a ID.

What could have an ID would be the the addr_to, addr_cc etc. menuitems. I don't see what a extension could change here, but when you are on adding IDs, you can also add theme here.

Comment 3

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Addressed feedback. Thanks Richard!

Do you happen to know you could review this change?

Comment 4

[Richard Marti (:Paenglab)]

I think I could review this, but it would be better if you choose a more experienced XUL reviewer like Magnus.

Comment 5

[Magnus Melin [:mkmelin]]

The sub elements in addressCol1#1 (and the kind)  shouldn't have ids, as you'd end up with duplicate ids when new rows are created.

Comment 6

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Fixed that child-ID issue and flagging Magnus for review.

Comment 7

[Josiah Bruner [:jsbruner]]

Magnus, will you be able to get to this soon? If not, I'm sure Richard could take over the review.

Comment 8

[Thomas D. (:thomas8)]

Comment on attachment 770958 [details] [diff] [review]
Patch.

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

Disclaimer: This is not an official review, just using splinter for feedback :)

Josiah, thanks for picking this up and making menuitems accessible for addons by adding IDs. As you started out from "my" bug 521158, I got interested and had a look. Unfortunately, I'm not aware of a style guide for this, so everybody seems to use their own naming patterns. Nevertheless, from looking at some other examples in the code, I'd like to suggest some improvements (up for discussion).

::: mail/components/compose/content/messengercompose.xul
@@ +331,5 @@
>  
>  <menupopup id="msgComposeAttachmentItemContext"
>             onpopupshowing="updateAttachmentItems();">
> +  <menuitem id="msgComposeAttachmentOpenItem"
> +            label="&openAttachment.label;"

Imo this ID feels odd because it's so structurally similar to a function or command name, while not providing hints about the context menu it belongs to, nor the exact string of the command it triggers.

Perhaps this would be clearer and more systematical:

<menuitem id="msgComposeAttachmentItemContext_openAttachment"

- use exact ID of parent context menu
- use underscore (_) for easier visual parsing
- use exact substring of command id
- menuitem is implicit as we are on a context menu ("msgComposeAttachmentItemContext_openAttachment_menuitem" might be a bit too long while not adding much value)

By analogy for subsequent menuitems:

<menuitem id="msgComposeAttachmentItemContext_delete"

<menuitem id="msgComposeAttachmentItemContext_renameAttachment"

<menu id="msgComposeAttachmentItemContext_convertCloud"

@@ +348,4 @@
>          accesskey="&convertCloud.accesskey;"
>          command="cmd_convertCloud">
> +    <menupopup id="convetCloudMenuItemsPopup"
> +               onpopupshowing="addConvertCloudMenuItems(this, 'context_convertCloudSeparator', 'context_convertCloud');">

Wrong spelling, and let's change structure, too:

<menupopup id="msgComposeAttachmentItemContext_convertCloud_popup"

@@ +371,5 @@
>             onpopupshowing="updateAttachmentItems();">
>    <menuitem label="&selectAll.label;"
>              accesskey="&selectAll.accesskey;"
>              command="cmd_selectAll"/>
>    <menuseparator/>

I don't see why we should skip selectAll menuitem and the menuseparator while we're practically wiring up the whole rest of the menu with IDs

<menuitem id="msgComposeAttachmentListContext_selectAll"

<menuseparator id="msgComposeAttachmentListContext_selectAll_separator"/>

@@ +378,2 @@
>              accesskey="&attachFile.accesskey;"
>              command="cmd_attachFile"/>

Again, imo these id's are much too similar to typical id's of a command, and Item is ambiguous because it's not separated.

I think this would be clearer and more systematical, same principle as above:

<menuitem id="msgComposeAttachmentListContext_attachFile"

<menu id="msgComposeAttachmentListContext_attachCloud"

<menupopup id="msgComposeAttachmentListContext_attachCloud_popup"

<menuitem id="msgComposeAttachmentListContext_attachPage"

@@ +703,2 @@
>                    accesskey="&attachFileCmd.accesskey;"
>                    command="cmd_attachFile"/>

Imo using list_ in the ID is too fuzzy and confusing here. Let's try more explicit & systematical structure as above:

<menuitem id="button-attachPopup_attachFile"

<menu id="button-attachPopup_attachCloud"

<menuitem id="button-attachPopup_attachPage"

<menuitem id="button-attachPopup_attachVCard"

@@ +739,1 @@
>                       command="cmd_saveAsFile" type="radio" name="radiogroup_SaveAs"/>

<menuitem id="button-savePopup_saveAsFile"

<menuitem id="button-savePopup_saveAsDraft"

<menuitem id="button-savePopup_saveAsTemplate"

Comment 9

[Thomas D. (:thomas8)]

Comment on attachment 770958 [details] [diff] [review]
Patch.

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

Disclaimer: This is not an official review, just using splinter for feedback :)

Sorry, previous feedback of comment 8 went wrong because I had wrong assumptions about splinter's context-providing behaviour, so here's the same feedback again, now including the right lines:

Josiah, thanks for picking this up and making menuitems accessible for addons by adding IDs. As you started out from "my" bug 521158, I got interested and had a look. Unfortunately, I'm not aware of a style guide for this, so everybody seems to use their own naming patterns. Nevertheless, from looking at some other examples in the code, I'd like to suggest some improvements (up for discussion).

::: mail/components/compose/content/messengercompose.xul
@@ +331,5 @@
>  
>  <menupopup id="msgComposeAttachmentItemContext"
>             onpopupshowing="updateAttachmentItems();">
> +  <menuitem id="msgComposeAttachmentOpenItem"
> +            label="&openAttachment.label;"

Imo this ID feels odd because it's so structurally similar to a function or command name, while not providing hints about the context menu it belongs to, nor the exact string of the command it triggers.

Perhaps this would be clearer and more systematical:

<menuitem id="msgComposeAttachmentItemContext_openAttachment"

- use exact ID of parent context menu
- use underscore (_) for easier visual parsing
- use exact substring of command id
- menuitem is implicit as we are on a context menu ("msgComposeAttachmentItemContext_openAttachment_menuitem" might be a bit too long while not adding much value)

By analogy for subsequent menuitems:

<menuitem id="msgComposeAttachmentItemContext_delete"

<menuitem id="msgComposeAttachmentItemContext_renameAttachment"

<menu id="msgComposeAttachmentItemContext_convertCloud"

@@ +348,3 @@
>          accesskey="&convertCloud.accesskey;"
>          command="cmd_convertCloud">
> +    <menupopup id="convetCloudMenuItemsPopup"

Wrong spelling, and let's change structure, too:

<menupopup id="msgComposeAttachmentItemContext_convertCloud_popup"

@@ +371,5 @@
>             onpopupshowing="updateAttachmentItems();">
>    <menuitem label="&selectAll.label;"
>              accesskey="&selectAll.accesskey;"
>              command="cmd_selectAll"/>
>    <menuseparator/>

I don't see why we should skip selectAll menuitem and the menuseparator while we're practically wiring up the whole rest of the menu with IDs

<menuitem id="msgComposeAttachmentListContext_selectAll"

<menuseparator id="msgComposeAttachmentListContext_selectAll_separator"/>

@@ +373,5 @@
>              accesskey="&selectAll.accesskey;"
>              command="cmd_selectAll"/>
>    <menuseparator/>
> +  <menuitem id="cmdAttachFileItem"
> +            label="&attachFile.label;"

Again, imo these id's are much too similar to typical id's of a command, and Item is ambiguous because it's not separated.

I think this would be clearer and more systematical, same principle as above:

<menuitem id="msgComposeAttachmentListContext_attachFile"

<menu id="msgComposeAttachmentListContext_attachCloud"

<menupopup id="msgComposeAttachmentListContext_attachCloud_popup"

<menuitem id="msgComposeAttachmentListContext_attachPage"

@@ +698,5 @@
>                 ondragdrop="nsDragAndDrop.drop(event, envelopeDragObserver);"
>                 ondragexit="nsDragAndDrop.dragExit(event, envelopeDragObserver);">
>        <menupopup id="button-attachPopup" onpopupshowing="updateAttachmentItems();">
> +        <menuitem id="list_attach_file"
> +                  label="&attachFileCmd.label;"

Imo using list_ in the ID is too fuzzy and confusing here. Let's try more explicit & systematical structure as above:

<menuitem id="button-attachPopup_attachFile"

<menu id="button-attachPopup_attachCloud"

<menuitem id="button-attachPopup_attachPage"

<menuitem id="button-attachPopup_attachVCard"

@@ +733,5 @@
>                 id="button-save" label="&saveButton.label;"
>                 tooltiptext="&saveButton.tooltip;"
>                 command="cmd_saveDefault">
>           <menupopup id="button-savePopup" onpopupshowing="InitFileSaveAsMenu();">
> +           <menuitem id="list_saveAs_file"

<menuitem id="button-savePopup_saveAsFile"

<menuitem id="button-savePopup_saveAsDraft"

<menuitem id="button-savePopup_saveAsTemplate"

Comment 10

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Updated patch. Details explained in next comment...

Comment 11

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Whoops, forgot something.

Comment 12

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Gah, forgot something else. Sorry for the bug spam guys!

Thomas, I went ahead with many of your changes. Although I did modify the naming structure a little bit from what you suggested to be shorter and more consistent with the existing scheme.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 771783 [details] [diff] [review]
Patch.

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

Looks good, r=mkmelin with a few things changed

::: mail/components/compose/content/messengercompose.xul
@@ +361,5 @@
>              label="&cancelUpload.label;"
>              accesskey="&cancelUpload.accesskey;"
>              command="cmd_cancelUpload"/>
>    <menuseparator/>
> +  <menuitem id="context_selectAll"

this could be something less generic

@@ +715,5 @@
>                    accesskey="&attachPageCmd.accesskey;"
>                    command="cmd_attachPage"/>
>          <menuseparator/>
> +        <menuitem id="attachPopup_attachVCard"
> +                  type="checkbox" label="&attachVCardCmd.label;"

please put label on a new line

@@ +836,5 @@
>                <listcol id="typecol-addressingWidget" style="&headersSpace.style;"/>
>                <listcol id="textcol-addressingWidget" flex="1"/>
>              </listcols>
>              <listitem class="addressingWidgetItem" allowevents="true">
> +              <listcell id="addressingWidgetCell1" class="addressingWidgetCell" align="stretch">

i don't think these should have an id either (likely cloned)

@@ +850,5 @@
>                      <menuitem value="addr_followup" label="&followupAddr.label;"/>
>                    </menupopup>
>                  </menulist>
>                </listcell>
> +              <listcell id="addressingWidgetCell2" class="addressingWidgetCell">

same here, class is enough

Comment 14

[Josiah Bruner [:jsbruner]]

(In reply to Magnus Melin from comment #13)
> Comment on attachment 771783 [details] [diff] [review]
> 
> ::: mail/components/compose/content/messengercompose.xul
> @@ +361,5 @@
> >              label="&cancelUpload.label;"
> >              accesskey="&cancelUpload.accesskey;"
> >              command="cmd_cancelUpload"/>
> >    <menuseparator/>
> > +  <menuitem id="context_selectAll"
> 
> this could be something less generic

Do you have a suggestion for this? context_selectAll is 100% consistent with other menuitems in this section. E.G. context_cancelUpload, context_convertAttachment.

Comment 15

[Magnus Melin [:mkmelin]]

Ok lets you what was in the patch

Comment 16

[Thomas D. (:thomas8)]

Comment on attachment 771783 [details] [diff] [review]
Patch.

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

Disclaimer: This is not a review, just feedback.

(In reply to Josiah Bruner [:JosiahOne] from comment #12)
> Created attachment 771783 [details] [diff] [review]
> Thomas, I went ahead with many of your changes. Although I did modify the
> naming structure a little bit from what you suggested to be shorter and more
> consistent with the existing scheme.

If you could explain the "existing scheme" to me, that would be great; I don't see consistent existing pattern.
I understand the desire for shorter id's; I'm not sure if shortness is an advantage when we lose context info.

Patch is going in good direction, but I did not fully understand "your" scheme and I still see some confusing inconsistencies in your patch...

::: mail/components/compose/content/messengercompose.xul
@@ +330,5 @@
>  </menupopup>
>  
>  <menupopup id="msgComposeAttachmentItemContext"
>             onpopupshowing="updateAttachmentItems();">
> +  <menuitem id="msgComposeAttachment_openItem"

While we might get away with this, this still suffers from the problems I mentioned before:

- no clue that this is part of a context menu
- openItem is ambiguous:
  - does it refer to an action (command) of opening attachment item? -> then you should use correct command name without cmd_ instead
  - does it refer to the *menu*item that triggers *openAttachment* function? then it should at least have an underscore before _item, or _menuitem.
- if you want shorter id, imo it would be better to omit msgCompose (because that's known, you can only call getElementById for a known xul document which is messengercompose.xul in this case) and to make the specific context explicit instead:

id="attachmentItemContext_openAttachment"

it's actually helpful to use "attachmentItemContext_" because this context menu applies only to attachment items but not to the attachment list.
Compare "mailContext_someaction" where "mailContext" is the ID of the mailContext popup. And Lightning even adds "_menuitem" no matter what. That's a scheme there...

Or just use full reference:

"msgComposeAttachmentItemContext_openAttachment"

I don't see problem with long id's if they are logically structured and thus more unique and less error-prone. Addons will mostly only use the id once when they overlay.

Same applies for all of the following menuitems of the same level.

@@ +348,3 @@
>          accesskey="&convertCloud.accesskey;"
>          command="cmd_convertCloud">
> +    <menupopup id="context_convertCloudMenuItemsPopup"

??? Well, very generally speaking, this is still on the attachment item context menu, but actually this is on its own popup which is second level compared to the main attachment item context menu.

I find this "scheme" of NOT using "context_" for menuitems that actually *belong* to the first level context menu, but using "context_" for other items which are on a secondary level *flyout popup* (which in itself is not a context menu) somewhat irritating...

Also, what is the intended meaning of "convertCloudMenuItemsPopup"? are we converting menu items? converting the popup? Perhaps this is missing some underscores? "convertCloud _menu_popup" or "convertCloud_menuItems_popup"?

@@ +356,5 @@
>                  command="cmd_convertAttachment"/>
>        <menuseparator id="context_convertCloudSeparator"/>
>      </menupopup>
>    </menu>
>    <menuitem id="context_cancelUpload"

that looks *very* inconsistent to me.
correct me if I'm wrong, but judging from indentation, this menuitem is exactly on the same level as the "msgComposeAttachment_openItem" menuitem above (per your proposed ids). So to be consistent with your scheme, you'd have to use:

msgComposeAttachment_cancelUpload

Also in your logic, why does this not have "Item" appended?

...or with my compromise proposal above in this review:

attachmentItemContext_cancelUpload

@@ +361,5 @@
>              label="&cancelUpload.label;"
>              accesskey="&cancelUpload.accesskey;"
>              command="cmd_cancelUpload"/>
>    <menuseparator/>
> +  <menuitem id="context_selectAll"

Magnus said:
> this could be something less generic

Translation: the term "context" in the id is *too general* and does not give clues which "context" menu this refers to:

- context menu with id="msgComposeAttachmentItemContext"
OR
- context menu with id="msgComposeAttachmentListContext" ?

Iow, in line with what I'm saying, Magnus asked you to use something more *unique* here to make the specific context of this very context menu more explicit. The id of this context menu is "msgComposeAttachmentItemContext", so that's still the best way of explaining where these menuitems belong, or at least the shorter compromise "attachmentItemContext" I propose above. As we have several different and additionally nested context menus here, it certainly helps to keep them apart when naming ids of their children.

So imo this should be either

attachmentItemContext_selectAll

or

msgComposeAttachmentItemContext_selectAll

@@ +370,5 @@
>  
>  <menupopup id="msgComposeAttachmentListContext"
>             onpopupshowing="updateAttachmentItems();">
> +  <menuitem id="attachmentListContext_selectAll"
> +            label="&selectAll.label;"

This looks good, and has the same scheme which I suggested above as a compromise.
Note how this scheme is different from your above scheme, so the analogy between the two context menus is lost.

@@ +699,5 @@
>                 ondragover="nsDragAndDrop.dragOver(event, envelopeDragObserver);"
>                 ondragdrop="nsDragAndDrop.drop(event, envelopeDragObserver);"
>                 ondragexit="nsDragAndDrop.dragExit(event, envelopeDragObserver);">
>        <menupopup id="button-attachPopup" onpopupshowing="updateAttachmentItems();">
> +        <menuitem id="attachPopup_attachFile"

which "attachPopup"? Somewhere on attachment items or attachment list? why skip the relevant context information "button-..." to indicate that this is part of the attachment button on toolbar?

same for following items on this level

@@ +735,5 @@
>                 id="button-save" label="&saveButton.label;"
>                 tooltiptext="&saveButton.tooltip;"
>                 command="cmd_saveDefault">
>           <menupopup id="button-savePopup" onpopupshowing="InitFileSaveAsMenu();">
> +           <menuitem id="savePopup_saveAsFile"

not sure why "button-..." should be omitted, I suppose we do have another twin popup on the menus so this and following isn't precise enough

@@ +836,5 @@
>                <listcol id="typecol-addressingWidget" style="&headersSpace.style;"/>
>                <listcol id="textcol-addressingWidget" flex="1"/>
>              </listcols>
>              <listitem class="addressingWidgetItem" allowevents="true">
> +              <listcell id="addressingWidgetCell1" class="addressingWidgetCell" align="stretch">

> i don't think these should have an id either (likely cloned)

I believe we're only cloning rows, not columns, and this is about the column... but I didn't check.
If it's possible to have an ID here without breaking things, we should have it I think.

@@ +850,5 @@
>                      <menuitem value="addr_followup" label="&followupAddr.label;"/>
>                    </menupopup>
>                  </menulist>
>                </listcell>
> +              <listcell id="addressingWidgetCell2" class="addressingWidgetCell">

> same here, class is enough

suppose that depends on what exactly you want to do with your addon, with class only it's harder to differentiate between the first and second column...

Comment 17

[Thomas D. (:thomas8)]

> Also, what is the intended meaning of "convertCloudMenuItemsPopup"? are we
> converting menu items? converting the popup? Perhaps this is missing some
> underscores? "convertCloud _menu_popup" or "convertCloud_menuItems_popup"?

I'm not proposing these, just guessing the meaning.
Probably should become something like:

attachmentItemContext_convertCloud_popup

and then for the subitems perhaps something like

[attachmentItemContext_]convertCloud_convertAttachment

This one is trickier on the second level...

Comment 18

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Took some of what Thomas said and updated the patch. So Magnus, could you please briefly review this again? Only thing I changed is the names of the IDs and then what you said in your comment.

Thomas, thank you for your feedback. It appears that the scheme did become inconsistent. I am now quite satisfied with this and since I have many, many things to do, this will be my final version of the patch. (Unless Magnus has any changes for me)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/568d0d8f7300

Comment 20

[Mark Banner (:standard8)]

Backed out due to unit test failures:

https://hg.mozilla.org/comm-central/rev/0eabb14b8b24

Looked like some of these were because some ids were changed, and they weren't accounted for elsewhere.

Comment 21

[Josiah Bruner [:jsbruner]]

Attachment: Fix tests and dependent files

Fixes the test failures and probably potential future bugs. Try run proof: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=171e7852f0c7

Since Mike wrote the test that this broke, I'll let him review it. :) Should be very quick though.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 774417 [details] [diff] [review]
Fix tests and dependent files

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

These IDs are a tad verbose, but I can't come up with anything better.

I'll assume you've run the cloudfile tests and made sure they all pass. Assuming that, r=me. Thanks Josiah!

Comment 23

[Josiah Bruner [:jsbruner]]

Thanks Mike! Requesting checkin...

Verified results here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3c39366d7ac9

Comment 24

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/a7e0cc2171dc
https://hg.mozilla.org/comm-central/rev/b7359393d3fe