[de-xbl] Migrate scroll-container binding to custom element.

RESOLVED FIXED in 7.0

Status

enhancement
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(2 attachments, 14 obsolete attachments)

38.67 KB, image/png
Details
22.09 KB, patch
arshad
: review+
Details | Diff | Splinter Review
Assignee

Description

6 months ago
No description provided.
Assignee

Updated

6 months ago
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Assignee

Comment 1

6 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Assignee

Comment 2

6 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9027514 - Attachment is obsolete: true
Assignee

Updated

6 months ago
Attachment #9028136 - Flags: feedback?(mkmelin+mozilla)
Assignee

Comment 3

6 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9028136 - Attachment is obsolete: true
Attachment #9028136 - Flags: feedback?(mkmelin+mozilla)
Attachment #9028827 - Flags: feedback?(mkmelin+mozilla)
Assignee

Updated

6 months ago
Assignee

Comment 4

6 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9028827 - Attachment is obsolete: true
Attachment #9028827 - Flags: feedback?(mkmelin+mozilla)
Attachment #9028830 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9028830 [details] [diff] [review]
scroll-container.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
@@ +54,5 @@
>  /**
>   * Sets up the attendee dialog
>   */
>  function onLoad() {
> +    window.freebusyRowScrollContainer = Object.create(ScrollContainer)

these are really usuappy set as g (for global) variables instead of defining on window

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ +114,5 @@
>              <richlistitem class="freebusy-listitem"
>                            allowevents="true">
>                <hbox flex="1">
> +                <box class="scroll-container container"
> +                     anonid="container"

please remove the anonids

@@ +117,5 @@
> +                <box class="scroll-container container"
> +                     anonid="container"
> +                     flex="1"
> +                     style="overflow: hidden; clip: rect(0px 0px 0px 0px);">
> +                  <box class="content"

looks like wrong indent

::: calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml
@@ +553,5 @@
>        </property>
>  
>        <property name="scroll">
>          <setter><![CDATA[
> +            let offset = window.freebusyTimebarScrollContainer.x;

this in particular gets a bit ugly.

But, as we discussed, probably better to go with a custom element instead for this, and only load it for the event dialog.
Attachment #9028830 - Flags: review?(mkmelin+mozilla) → review-
Assignee

Updated

5 months ago
Summary: [de-xbl] Inline and Scriptify scroll-container binding. → [de-xbl] Migrate scroll-container binding to custom element.
Assignee

Comment 6

5 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9028830 - Attachment is obsolete: true
Attachment #9030067 - Flags: review?(makemyday)
Attachment #9030067 - Flags: feedback?(mkmelin+mozilla)
Assignee

Comment 7

5 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9030067 - Attachment is obsolete: true
Attachment #9030067 - Flags: review?(makemyday)
Attachment #9030067 - Flags: feedback?(mkmelin+mozilla)
Attachment #9030072 - Flags: review?(makemyday)
Attachment #9030072 - Flags: feedback?(mkmelin+mozilla)
Assignee

Comment 8

5 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9030072 - Attachment is obsolete: true
Attachment #9030072 - Flags: review?(makemyday)
Attachment #9030072 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9030075 [details] [diff] [review]
scroll-container.patch

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

Appears to work
Attachment #9030075 - Flags: feedback+
Assignee

Updated

5 months ago
Attachment #9030075 - Flags: review?(makemyday)

Comment 10

3 months ago
Comment on attachment 9030075 [details] [diff] [review]
scroll-container.patch

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

This patch needs a rebase for jar.mn if you have the patch from bug 1511229 applied (maybe it would make sense to define an order for the calendar de-xbl bugs with pending reviews and rebase its patches if required).

Apart from that, there are several issues with this patch:

Horizontal scrolling:
- if click the arrows of the horizontal scroll bar, just the scroll bar position indicator moves, but the content is not moved
- if you move the horizontal scroll bar position indicator to the right, the dates at the top of the are not getting updated (while they are if you use the previous slot button)

Vertical scrolling:
- if putting the focus to the attendee list and then press tab until you exceed the already visible attendee rows, the attendee list gets new child nodes up to the bottom of the dialog instead of starting to display a vertical scroll bar when you have more attendee list entries then would fit for the free-busy grid

So r- to get that fixed. Please also consider the below comments when updating the patch.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-bindings.js
@@ +1,1 @@
> +class MozScrollContainer extends MozXULElement {

Please add a license block to the top of the file.

Also, please add JSDOC descriptions for MozScrollContainer and its functions/getters/setters.

::: calendar/base/content/dialogs/calendar-event-dialog.css
@@ +35,2 @@
>      -moz-user-focus: normal;
>  }

Why is this still needed? I would assume we can remove the scroll-container block here.

@@ +38,5 @@
> +scroll-container .container {
> +    overflow: hidden;
> +    clip: rect(0px 0px 0px 0px);
> +}
> +

css files in calendar/base/content are not intended to define styles but only hold the binding definitions. Styling should go into calendar/base/themes/common or if OS specific into its sibbling folder. For this definition, calendar/base/themes/common/dialogs/calendar-event-dialog.css might be an appropriate venue, since we have already several definition for the attendee dialog therein.
Attachment #9030075 - Flags: review?(makemyday) → review-
Assignee

Comment 11

3 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9030075 - Attachment is obsolete: true
Attachment #9042940 - Flags: feedback+
Assignee

Comment 12

3 months ago

Apart from that, there are several issues with this patch:

Horizontal scrolling:

  • if click the arrows of the horizontal scroll bar, just the scroll bar
    position indicator moves, but the content is not moved
  • if you move the horizontal scroll bar position indicator to the right, the
    dates at the top of the are not getting updated (while they are if you use
    the previous slot button)

I am not sure what is horizontal scrollbar are you taling about. I dont see any scrollbar maybe because I am on macos.

Vertical scrolling:

  • if putting the focus to the attendee list and then press tab until you
    exceed the already visible attendee rows, the attendee list gets new child
    nodes up to the bottom of the dialog instead of starting to display a
    vertical scroll bar when you have more attendee list entries then would fit
    for the free-busy grid

So r- to get that fixed. Please also consider the below comments when
updating the patch.
that's attendee-list issue. the css that is used uses fixed positioning which messes up layot imo. I have already opened a bug 1525840 for that.

Assignee

Updated

3 months ago
Blocks: 1512805
Assignee

Updated

3 months ago
Blocks: 1511270

Comment 13

3 months ago

Screenshot with the scrollbar

Assignee

Comment 14

2 months ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9042940 - Attachment is obsolete: true
Assignee

Updated

2 months ago
No longer blocks: 1511270
Assignee

Comment 15

a month ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9050254 - Attachment is obsolete: true
Assignee

Comment 16

a month ago

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

This patch shoul probably work. I still have to check it on my ubuntu image.

Assignee

Updated

a month ago
Attachment #9056459 - Flags: review?(philipp)
Assignee

Comment 17

a month ago

Just verified that this patch works for ubuntu image as well adn I dont see the horzontal scrolling issue that Makemyday earlier talked about.

Comment on attachment 9056459 [details] [diff] [review]
scroll-container.patch

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

r+ with comments:

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +141,5 @@
>          return this.mEndDate;
>      }
>  
>      /**
> +     * Sets mDayOffset to a new value and adjust calendar-event-scroll-container children according to it.

Don't use internal names like mDayOffset. If this documentation is generated, nobody will know what that means.

Please describe what this will do to the view, not the code itself.
Attachment #9056459 - Flags: review?(philipp) → review+
Assignee

Comment 19

a month ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9056459 - Attachment is obsolete: true
Attachment #9058841 - Flags: review+
Assignee

Updated

a month ago
Keywords: checkin-needed

Comment 20

a month ago

Hmm, you changed let numChilds = container.childNodes.length; to container.content.childNodes.length; at the last minute. How is that motivated? That change didn't run on try.

Keywords: checkin-needed
Assignee

Comment 21

a month ago

(In reply to Jorg K (GMT+2) from comment #20)

Hmm, you changed let numChilds = container.childNodes.length; to container.content.childNodes.length; at the last minute. How is that motivated? That change didn't run on try.

Yes that was just left unnoticed and I realised it in the end that xbl elements doesn't count the anon elements as their child, so if you run xblElem.firstChild then the first child that's not anon element is picked up. I ll put another try then.

Magnus can you +1 me on this xbl child change that I added in last minute?

Flags: needinfo?(mkmelin+mozilla)
Assignee

Updated

a month ago
Keywords: checkin-needed
Assignee

Comment 23

a month ago

I ll rebase the patch if any other patch lands before this

Keywords: checkin-needed
Assignee

Comment 24

a month ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9058841 - Attachment is obsolete: true
Attachment #9059103 - Flags: review+
Assignee

Comment 25

a month ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9059103 - Attachment is obsolete: true
Attachment #9059106 - Flags: review+

Comment 26

a month ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/268b0132591a
Migrate scroll-container binding to custom element. r=philipp

Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED

Updated

a month ago
Target Milestone: --- → 7.0

Comment 27

a month ago

Hmm, checkin needed and then a rebased r+ patch seem like a pretty clear sign that this was ready. Apparently it wasn't, so here's the backout:

https://hg.mozilla.org/comm-central/rev/72eadf95a12007ee4a2c5d02951ffd7851612628

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 28

a month ago
Posted patch scroll-container.patch (obsolete) — Splinter Review
Attachment #9059106 - Attachment is obsolete: true
Attachment #9059648 - Flags: review+
Assignee

Comment 29

a month ago
Attachment #9059648 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #9059649 - Flags: review+
Assignee

Updated

a month ago
Keywords: checkin-needed
Assignee

Comment 30

a month ago

To be applied on the top of freebusy-day patch - Bug 1511270.

Comment 32

a month ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/01f7fa783762
Migrate scroll-container binding to custom element. r=philipp

Status: REOPENED → RESOLVED
Last Resolved: a month agoa month ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.