351445 - Match all messages in the filter and save search dialog hides UI elements

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Scott MacGregor]

We shouldn't be hiding the filter rules when you choose Match all messages.

We should just disable the rules box.

Comment 1

[Žiga Sancin]

Attachment: disable listbox

I'm not sure if that's the right approach here, but it seems that there's no easier way to do it.

Comment 2

[Scott MacGregor]

Comment on attachment 264179 [details] [diff] [review]
disable listbox

That turned out to be more complicated than I thought it would :)

Looks ok to me, asking Magnus for a peer review too.

Comment 3

[Žiga Sancin]

Thanks Scott. 

By the way, this patch also fixes the save search dialog, which uses the same xul overlay file.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 264179 [details] [diff] [review]
disable listbox

You should enable disabled elements again if the match all is deselected. Now you get stuck with a disabled input field when you toggle between match all of/any/all. 

I think JavaDoc syntax ( /** */ ) is preferable for function documentation, not that that file uses it much...

Comment 5

[Žiga Sancin]

Good catch! Actually, the text input field gets enabled, but it's not focusable anymore. I'll look into that, thanks for the review.

Comment 6

[Žiga Sancin]

Attachment: disable listbox, v2

Weird, just switching disabled attributes doesn't work for searchvalue textboxes. We also need to remove them.

I've also added java doc style comments and a tiny change to mailWidgets (I forgot to add xbl:inherits to the second xul:textbox element in the searchvalue binding).

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 272325 [details] [diff] [review]
disable listbox, v2

>+ * @param matchAllValue
>+ *        boolean value from the first search term

Please put it on the same line.

>+      searchValueList[i].setAttribute("disabled", matchAllValue);
>+      if (!matchAllValue)
>+        searchValueList[i].removeAttribute("disabled");

Are both needed? I would have thought just removing the attribute was enough.

Looks quite good, but I noticed it doesn't disable the value dropdowns for Priority and Status.

Comment 8

[Žiga Sancin]

Comment on attachment 272325 [details] [diff] [review]
disable listbox, v2

Oops, I'll post a new patch.

Comment 9

[Žiga Sancin]

Attachment: disable listbox, v2.1

I've added a few more xbl:inherits. :)

Regarding your question about using setAttribute and removeAttribute: I think in this case they are needed both, because a searchvalue element doesn't reset the value of the disabled attribute if only removeAttribute is used.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 272343 [details] [diff] [review]
disable listbox, v2.1

Looks good now, thx!
r=mkmelin+mozilla

Comment 11

[Žiga Sancin]

Comment on attachment 272343 [details] [diff] [review]
disable listbox, v2.1

After numerous complications, I think this patch is ready to go. :) Checking for a sr again.

Comment 12

[Scott MacGregor]

Comment on attachment 272343 [details] [diff] [review]
disable listbox, v2.1

yeah..bisi is back. looks good to me.

Comment 13

[Žiga Sancin]

I'd like to throw a new chicken into the tree. Yeah, this means you, attachment 272343 [details] [diff] [review]. :)

Comment 14

[Nickolay_Ponomarev]

mozilla/mailnews/base/resources/content/mailWidgets.xml              1.118
mozilla/mailnews/base/search/resources/content/searchTermOverlay.js  1.49

Comment 15

[Žiga Sancin]

Thanks Nickolay!

Comment 16

[[e-mail]]

(In reply to comment #6)
>Weird, just switching disabled attributes doesn't work for searchvalue
>textboxes. We also need to remove them.
That's because they're really HTML <input> elments in disguise, and they disable themselves simply on the presence of a disabled attribute.