1518191 - [de-xbl] De-XBL view-tab

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1484976 +++

De-xbl tab-view

    Found at https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-widgets.xml#477
    Hooked up here: https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-widget-bindings.css#40
    Affecting (at least): https://searchfox.org/comm-central/source/calendar/base/content/calendar-views.xul#69

InheritAttribute is likely of interest - https://searchfox.org/comm-central/search?q=inheritAttribute&path=

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: bug1518191_De-XBL_view-tab.patch

I moved out the CSS logic of Selected/Unselected Labels to radio wrapper as radio is still XML binding so was not able to inherit property "selected".

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9035588 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

Looks like a good start!

::: calendar/base/content/calendar-tab-view.js
@@ +8,5 @@
> +
> +class MozViewTab extends MozXULElement {
> +    connectedCallback() {
> +        if (this.delayConnectedCallback()) {
> +            return;

I don't think you need this?

@@ +10,5 @@
> +    connectedCallback() {
> +        if (this.delayConnectedCallback()) {
> +            return;
> +        }
> +        this.textContent = "";

nor this?

@@ +12,5 @@
> +            return;
> +        }
> +        this.textContent = "";
> +        this.appendChild(MozXULElement.parseXULToFragment(`
> +            <hbox class="tab-middle box-inherit" inherits="align,dir,pack,orient" flex="1">

I assume this is all coming from tabbox.xml, but for this use case perhaps we can remove some stuff that's never going to be used. 
As you can see, the only attribute ever set on the view-tab is label. All the other stuff I think can be removed. (= no inherit needed here)

@@ +13,5 @@
> +        }
> +        this.textContent = "";
> +        this.appendChild(MozXULElement.parseXULToFragment(`
> +            <hbox class="tab-middle box-inherit" inherits="align,dir,pack,orient" flex="1">
> +                <image class="tab-icon" inherits="validate,src=image"></image>

This tab also never has any image so we can remove that

@@ +14,5 @@
> +        this.textContent = "";
> +        this.appendChild(MozXULElement.parseXULToFragment(`
> +            <hbox class="tab-middle box-inherit" inherits="align,dir,pack,orient" flex="1">
> +                <image class="tab-icon" inherits="validate,src=image"></image>
> +                <label class="tab-text" inherits="value=label,accesskey,crop,disabled" flex="1"></label>

none of the inherited attributes (apart from the actual label value) is ever set. Here too looks like we can simplify

@@ +17,5 @@
> +                <image class="tab-icon" inherits="validate,src=image"></image>
> +                <label class="tab-text" inherits="value=label,accesskey,crop,disabled" flex="1"></label>
> +            </hbox>
> +        `));
> +        this._updateAttributes();

You probably want to add these

static get observedAttributes
attributeChangedCallback

... so observe label (only). Not that it ever changes in current use.

@@ +43,5 @@
> +            this.inheritAttribute(tabText, "disabled");
> +        }
> +    }
> +}
> +customElements.define("view-tab", MozViewTab);

maybe we can make this named calendar-view-tab

::: calendar/base/themes/osx/calendar-views.css
@@ +70,4 @@
>      padding-right: 0;
>      margin-bottom: 0;
>      position: static;
> +    margin: 0;

needed elsewhere too? (in shared/ theme?)
I notice at least on linux there's now a slight gap above the tab

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: bug1518191_De-XBL_view-tab.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9035857 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

Regarding the css, please compare the view for nightly and with your patch applied. Above Day | Week | Multiweek | Month, there is still the slight gap. On nightly the tab goes all the way up to the border

::: calendar/base/content/calendar-tab-view.js
@@ +13,5 @@
> +
> +    connectedCallback() {
> +        this.appendChild(MozXULElement.parseXULToFragment(`
> +            <hbox class="tab-middle box-inherit" flex="1">
> +                <label class="tab-text" inherits="value=label" flex="1"></label>

the inherits= you can remove as it would do nothing anymore

@@ +15,5 @@
> +        this.appendChild(MozXULElement.parseXULToFragment(`
> +            <hbox class="tab-middle box-inherit" flex="1">
> +                <label class="tab-text" inherits="value=label" flex="1"></label>
> +            </hbox>
> +        `));

You could also just build the dom manually as this is simple content where no xbl bindings are used. If there are xbl binding in the content, then you must use parseXULToFragment

@@ +28,5 @@
> +        let tabText = this.querySelector(".tab-text");
> +
> +        if (this.hasAttribute("label")) {
> +            this.inheritAttribute(tabText, "value=label");
> +        }

for completeness, we've should also handle the case for !this.hasAttribute -> remove it instead. Well, in this case, better to just set label value to "" then.

I see now this is all a rather simple class. There's not really a need to use the inheritsAttribute, you can just do something like

* in connectedCallback save this._label =  , then
* in _updateAttributes: this._label.setAttribute("value", this.getAttribute("label"));

::: calendar/base/content/calendar-views.xul
@@ +15,5 @@
>  
>  <overlay id="calendar-views-overlay"
>           xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <!-- Javascript includes -->

you can remove this comment

@@ +71,1 @@
>                   calview="day"

make this a class instead

::: calendar/base/themes/osx/calendar-views.css
@@ +79,2 @@
>  
>  radio[calview][selected="true"] {

radio[calview] should now be changed to radio.calview (make it a class name).

Perhaps it needs more updates, does js code use it? Didn't check.

Comment 5

[Magnus Melin [:mkmelin]]

Later when they convert the radio.xml binding, this should likely get converted to become <radio is="calendar-view-tab">, but the conversion you're looking into would make that easier

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: bug1518191_De-XBL_view-tab.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9036428 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

Yep, this works and looks good to me!
/calendar likes to do their own reviews so forwarding the official review to Philipp

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9036428 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

I'm assuming you tested this, I have dark recollection that the calview attribute is used in other code, but if it works as is r=philipp.

Please make sure you run the linter before checkin.

::: calendar/base/content/calendar-tab-view.js
@@ +32,5 @@
> +    _updateAttributes() {
> +        if (this.hasAttribute("label")) {
> +            this._label.setAttribute("value", this.getAttribute("label"));
> +        }
> +        else {

This should fail the linter, it needs to be } else {.

Also, you could just shorthand:

this._label.setAttribute(this.getAttribute("label") || "");

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: bug1518191_De-XBL_view-tab.patch

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: bug1518191_De-XBL_view-tab.patch

The previous patch was missing a file. I have added it.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9037854 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

The point of the stack with selected-text and unselected-text was that the width of the text should not twitch where the selected font weight makes the text longer than the unselected style. I don't see any specific code to handle this, does it still work?

Given the low complexity of the binding, instead of having a view tab custom element it might make sense to instead have a custom element that would compensate for the difference between bold and not bold. Something like:

<label is="moz-bold-selection-label" selected="true">Day View</label>

There would be custom element css that adds a stack like the old way, or if possible just overlapping html elements that have visibility:hidden according to selected status.

I believe this could be re-used in other places, I think the minimonth for example has a similar construct.

Happy to explain in more detail on IRC if that sounded confusing. I'm going to r- for now because I'd like to take a look at the follow-up patch.

::: calendar/base/content/calendar-tab-view.js
@@ +3,5 @@
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/* global MozXULElement */

I'm wondering if it makes sense to just add MozXULElement to the globals for calendar/base/content/. Possibly something for a followup bug if other files in the directory are already using the comment.

Comment 12

[Khushil Mistry [:khushil324]]

Radio is still a XBL binding so I am finding it difficult to inherit the property "selected" to the label component. So currently, I have implemented labels with stack logic in the existing custom element. Is it fine?

I'm wondering if it makes sense to just add MozXULElement to the globals for
calendar/base/content/. Possibly something for a followup bug if other files
in the directory are already using the comment.

Should I put this comment in calendar-view.js?

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Khushil Mistry from comment #12)

Radio is still a XBL binding so I am finding it difficult to inherit the property "selected" to the label component. So currently, I have implemented labels with stack logic in the existing custom element. Is it fine?

I'm not quite sure how this will look. The original intent was to have the parent propagate the selected attribute to the label. As for your IRC question on where the CSS should go I think its best to discuss with Arshad and Magnus. I don't know where they have been putting CSS for custom elements.

Other elements using this behavior, I am not sure. I had the impression the date/timepickers use this. You'll need to check for other elements using a stack in a similar way to here. It is not impossible though that this is the only caller and I'm misremembering. Best check that first.

I'm wondering if it makes sense to just add MozXULElement to the globals for
calendar/base/content/. Possibly something for a followup bug if other files
in the directory are already using the comment.

Should I put this comment in calendar-view.js?

Rather in calendar/base/content/.eslintrc.js, adding it as a global. https://eslint.org/docs/user-guide/configuring#specifying-globals

Comment 14

[Khushil Mistry [:khushil324]]

Other elements using this behavior, I am not sure. I had the impression the date/timepickers use this. You'll need to check for other elements using a stack in a similar way to here. It is not impossible though that this is the only caller and I'm misremembering. Best check that first.

I found this binding using the same behaviour : https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-widgets.xml#26-35. I didn't find any other bindings using the label with stack.

Comment 15

[Magnus Melin [:mkmelin]]

Re adding MozXULElement to .eslint: core hasn't added it so far, so might be best to keep including one-off global statements IMHO.

Note that this binding will likely need to change (can change!) once they de-xbl <radio>. Then it should probably be a customzied built-in yes. But it's easier to finish here first since it was looking way more complex than it turned out to be.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Magnus Melin [:mkmelin] from comment #15)

Note that this binding will likely need to change (can change!) once they de-xbl <radio>. Then it should probably be a customzied built-in yes. But it's easier to finish here first since it was looking way more complex than it turned out to be.

Ok. Kushil, based on what you have at the moment let's go with the path that is the least amount of work. I think this makes most sense if the binding is going to change anyway.

Comment 17

[Khushil Mistry [:khushil324]]

Okay. I have implemented stack logic in current custom component and its working fine i.e. it is not changing the width. I am submitting that patch. You can review that.

Comment 18

[Khushil Mistry [:khushil324]]

Attachment: bug1518191_De-XBL_view-tab.patch

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9038229 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

Yep works fine. 
I find it  strange the css for this widget is spread out over calendar-widgets.css and calendar-views.css, but it's prior art... Semantically might be worth moving them all to the -widget.css file?

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9038229 [details] [diff] [review]
bug1518191_De-XBL_view-tab.patch

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

::: calendar/base/content/calendar-tab-view.js
@@ +5,5 @@
> +"use strict";
> +
> +/* global MozXULElement */
> +
> +class MozViewTab extends MozXULElement {

Let's add some docs here.

::: calendar/base/themes/common/calendar-views.css
@@ +896,5 @@
>      border-bottom: 1px solid var(--calview-border-bottom-color);
>  }
>  
> +.calview-tabs {
> +    margin: 0px;

No need for a unit when using 0. Changing this also makes it consistent with the remaining file. More info for reasonsing here: https://github.com/CSSLint/csslint/wiki/Disallow-units-for-zero-values

Comment 21

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_view-tab.patch

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9043058 [details] [diff] [review]
De-XBL_view-tab.patch

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

::: calendar/base/content/calendar-tab-view.js
@@ +6,5 @@
> +
> +/**
> + * Tab options that is displayed above the calender-view.
> + * It is used to navigate between differnet calender-views.
> + * @extends MozXULElement

Empty line between description and jsdoc directives

@@ +9,5 @@
> + * It is used to navigate between differnet calender-views.
> + * @extends MozXULElement
> + */
> +
> +class MozViewTab extends MozXULElement {

No empty line after jsdoc comment

Comment 23

[Khushil Mistry [:khushil324]]

Attachment: de_xbl_view-tab.patch

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: Bug1519800-de_xbl_view-tab.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=99561d93228e48aeed449033d84aef6a81985236

Comment 25

[Magnus Melin [:mkmelin]]

Sorry, but I see now bug 1495861 landed now, so let's make this <radio is="calendar-view-tab"> now in one shot.

Comment 26

[Khushil Mistry [:khushil324]]

Sure, I will update the patch.

Comment 28

[Khushil Mistry [:khushil324]]

Attachment: Bug-1518191-de_xbl_view-tab.patch

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9056716 [details] [diff] [review]
Bug-1518191-de_xbl_view-tab.patch

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

Looking through this, and the patch from bug 1542670, I think that could be preferable. That is css only - but not yet perfect as the tab border doesn't go up to the edge.

::: calendar/base/content/calendar-tab-view.js
@@ +5,5 @@
> +"use strict";
> +
> +/**
> + * Tab options that is displayed above the calender-view.
> + * It is used to navigate between differnet calender-views.

typo: different

also, "calendar views."

@@ +7,5 @@
> +/**
> + * Tab options that is displayed above the calender-view.
> + * It is used to navigate between differnet calender-views.
> + *
> + * @extends {customElements.get("radio")}

@extends {MozRadio}

same below

@@ +42,5 @@
> +        this.appendChild(this._hbox);
> +        this.initializeAttributeInheritance();
> +    }
> +}
> +customElements.define("calendar-view-tab", MozViewTab, { extends: 'radio' });

double quotes

::: calendar/base/themes/common/calendar-views.css
@@ +941,4 @@
>      background-color: rgba(0,0,0,.1);
>  }
>  
> +radio.calview > .tab-middle > stack > .selected-text {

For these, it would probably make more sense to match on is="celendar-view-tab" since other radios wouldn't have these internals
(you'd have to setAttribute("is"... yourself)

Comment 30

[Khushil Mistry [:khushil324]]

Attachment: Bug-1518191-de_xbl_view-tab.patch

Can you confirm if there is a gap above the top border? I am not seeing it on Mac.

Comment 31

[Magnus Melin [:mkmelin]]

No gap with this (that is using the CE). I was referring to the patch from bug 1542670.

Comment 32

[Khushil Mistry [:khushil324]]

With the only CSS, It will create a problem mentioned in the comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1518191#c13.

Comment 33

[Khushil Mistry [:khushil324]]

Attachment: Bug-1518191_de-xbl_view-tab.patch

Comment 34

[Khushil Mistry [:khushil324]]

Attachment: Bug-1518191_de-xbl_view-tab.patch

I have updated the patch with CSS only.

Comment 35

[Magnus Melin [:mkmelin]]

Comment on attachment 9061181 [details] [diff] [review]
Bug-1518191_de-xbl_view-tab.patch

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

I think this is good. 
I tried looking very closely if there's the problem described in comment 11. If there is it must be very minor. At least I don't see it.

Comment 36

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9061181 [details] [diff] [review]
Bug-1518191_de-xbl_view-tab.patch

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

I thought the calview attribute was previously used for something other than styling, but it seems not. Code looks good, r= me

Comment 37

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b9345e5f3c4697148f17d7148eb0d1ad263fa291

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f371d902e618
[de-xbl] convert view-tab binding to custom element. r=philipp