1679820 - remove <deck> from lightning-item-iframe.xhtml

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Khushil Mistry [:khushil324]]

Remove <deck> from https://searchfox.org/comm-central/rev/1fa5ebe1384434e904b33bb7de8f0a3d6e8bfdc5/calendar/lightning/content/lightning-item-iframe.xhtml#437

Two children that can be set hidden/not as appropriate instead of setting selectedIndex on the deck.

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679820_de-deck-lightning-item-iframe-xhtml-0.patch

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9190335 [details] [diff] [review]
Bug-1679820_de-deck-lightning-item-iframe-xhtml-0.patch

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

::: calendar/lightning/content/lightning-item-iframe.js
@@ +1322,1 @@
>        if (notCustomRule) {

No need to declare a variable for this, just write directly in the condition.

::: calendar/lightning/content/lightning-item-iframe.xhtml
@@ +434,4 @@
>                          value="custom"/>
>              </menupopup>
>            </menulist>
> +          <hbox id="repeat-box">

Do we need this extra container now that the two elements are not dependent by it?

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679820_de-deck-lightning-item-iframe-xhtml-1.patch

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9190473 [details] [diff] [review]
Bug-1679820_de-deck-lightning-item-iframe-xhtml-1.patch

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

::: calendar/lightning/content/lightning-item-iframe.js
@@ +3947,1 @@
>      // First of all collapse the details text. If we fail to

This whole condition is a bit weird.
It seems that we're hiding the "repeat-details" element, but then also collapsing it, and then removing the collapse after some labels have been filled, but then we're once again selecting it to remove the collapse after the else callback.

Can you investigate this condition and streamline it a bit?

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679820_de-deck-lightning-item-iframe-xhtml-2.patch

I have updated it a bit. Added try and catch to prevent the confusion.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9190647 [details] [diff] [review]
Bug-1679820_de-deck-lightning-item-iframe-xhtml-2.patch

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

::: calendar/lightning/content/lightning-item-iframe.js
@@ +3969,1 @@
>      }

I'm not sure about the try catch.
The failed string generation might happen inside the `recurrenceRule2String` method, which already returns null.
And if it returns null we fallback to the "ruleTooComplex" string.
I think simply putting `repeatDetails.hidden = false;` after the detailsString variable is ready would be enough.
Did you try simulating this scenario where the "ruleTooComplex" string is used?

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679820_de-deck-lightning-item-iframe-xhtml-3.patch

Yes, I have tried that. Agree, repeatDetails.hidden = false; should work.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9191274 [details] [diff] [review]
Bug-1679820_de-deck-lightning-item-iframe-xhtml-3.patch

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

Looks good, thanks.

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9191274 [details] [diff] [review]
Bug-1679820_de-deck-lightning-item-iframe-xhtml-3.patch

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

Just a little tidy-up to do. Otherwise LGTM!

::: calendar/lightning/content/lightning-item-iframe.js
@@ +2840,3 @@
>  
>    let repeatMenu = document.getElementById("item-repeat");
>    let repeatValue = repeatMenu.selectedItem.getAttribute("value");

Let's collect repeat-untilDate and repeat-details here so we don't have to getElementById them in four places in this function.

@@ -2847,5 @@
>        enableElementWithLock("todo-has-entrydate", "repeat-lock");
>      }
>    } else if (repeatValue == "custom") {
> -    let lastRepeatDeck = repeatDeck.selectedIndex;
> -    repeatDeck.selectedIndex = 1;

I *think* it's okay to remove this, but I hope you've done some manual checking here.

Comment 10

[Khushil Mistry [:khushil324]]

(In reply to Geoff Lankow (:darktrojan) from comment #9)

I think it's okay to remove this, but I hope you've done some manual
checking here.

Yes, there is an if-else condition. I have simplified it if the execution goes to if statement, don't do anything and if it goes to else statement, change the visibilities of the respective elements.

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679820_de-deck-lightning-item-iframe-xhtml-4.patch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92cd2f444d15
remove <deck> XUL element from lightning-item-iframe.xhtml dialog. r=aleca,darktrojan