Closed Bug 1518191 Opened 6 years ago Closed 6 years ago

[de-xbl] De-XBL view-tab

Categories

(Calendar :: General, enhancement)

Product:
Calendar
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(1 file, 12 obsolete files)

Bug-1518191_de-xbl_view-tab.patch
(deleted), patch
Fallen
: review+
mkmelin
: feedback+
Details | Diff | Splinter Review
Attached patch bug1518191_De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review

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".

Attachment #9035588 - Flags: review?(mkmelin+mozilla)
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
Attachment #9035588 - Flags: review?(mkmelin+mozilla)
Attached patch bug1518191_De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9035588 - Attachment is obsolete: true
Attachment #9035857 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9035857 - Flags: review?(mkmelin+mozilla)

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

Attached patch bug1518191_De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9035857 - Attachment is obsolete: true
Attachment #9036428 - Flags: review?(mkmelin+mozilla)
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
Attachment #9036428 - Flags: ui-review+
Attachment #9036428 - Flags: review?(philipp)
Attachment #9036428 - Flags: review?(mkmelin+mozilla)
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") || "");
Attachment #9036428 - Flags: review?(philipp) → review+
Attached patch bug1518191_De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9036428 - Attachment is obsolete: true
Attachment #9037284 - Flags: review?(philipp)
Attached patch bug1518191_De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review

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

Attachment #9037284 - Attachment is obsolete: true
Attachment #9037284 - Flags: review?(philipp)
Attachment #9037854 - Flags: review?(philipp)
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.
Attachment #9037854 - Flags: review?(philipp) → review-

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?

(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

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.

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.

(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.

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.

Attached patch bug1518191_De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9037854 - Attachment is obsolete: true
Attachment #9038229 - Flags: review?(philipp)
Attachment #9038229 - Flags: feedback?(mkmelin+mozilla)
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?
Attachment #9038229 - Flags: feedback?(mkmelin+mozilla) → feedback+
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
Attachment #9038229 - Flags: review?(philipp) → review+
Attached patch De-XBL_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9038229 - Attachment is obsolete: true
Attachment #9043058 - Flags: review?(philipp)
Attachment #9043058 - Flags: feedback+
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
Attachment #9043058 - Flags: review?(philipp) → review+
Attached patch de_xbl_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9043058 - Attachment is obsolete: true
Attachment #9047513 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed

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

Keywords: checkin-needed

Sure, I will update the patch.

Attached patch Bug-1518191-de_xbl_view-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056374 - Attachment is obsolete: true
Attachment #9056716 - Flags: feedback?(mkmelin+mozilla)
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)
Attachment #9056716 - Flags: feedback?(mkmelin+mozilla)
Attached patch Bug-1518191-de_xbl_view-tab.patch (obsolete) (deleted) — — Splinter Review

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

Attachment #9056716 - Attachment is obsolete: true
Attachment #9056901 - Flags: feedback?(mkmelin+mozilla)

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

Attachment #9056901 - Flags: feedback?(mkmelin+mozilla)

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

Attached patch Bug-1518191_de-xbl_view-tab.patch (obsolete) (deleted) — — Splinter Review

I have updated the patch with CSS only.

Attachment #9056901 - Attachment is obsolete: true
Attachment #9061179 - Attachment is obsolete: true
Attachment #9061181 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9061181 - Flags: review?(philipp)
Attachment #9061181 - Flags: review?(mkmelin+mozilla)
Attachment #9061181 - Flags: feedback+
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
Attachment #9061181 - Flags: review?(philipp) → review+
Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: