Feature/enyo 4811 roysutton #1301

webOS101 · 2017-11-10T19:50:48Z

Issue Resolved / Feature Added

Marquee would not continue or start in certain situations. Additionally, Spottable was not
correctly forwarding onMouseEnter and onMouseLeave.

Resolution

Modify MarqueeDecorator to not start or stop its own marquee when controlled,
deferring that instead to MarqueeController. Additionally, keep track of hover status
in MarqueeDecorator when enabled so we can keep the status when switching to
disabled. MarqueeController is modified to add callbacks for enter and leave
so it can track the hover state of controls. Also, added a delay before stopping
controlled marquee so that moving among controls does not cause stuttering
marquee. Finally, added some code to PickerButton to trap hover so we can
start marquee when hovering disabled buttons.

Additional Considerations

We may need a new qa sample to demonstrate controlled marquees that use
marqueeOn focus and disabled marqueeOn focus components.

Links

ENYO-4811
ENYO-4882
ENYO-4907
ENYO-4910

Comments

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

viodragon2

Made some changes for LabeledItem. Other than that everything looks good to me 👍

aarontam

Some initial comments/questions. Taking a deeper dive still.

aarontam · 2017-11-15T22:21:43Z

packages/moonstone/Marquee/MarqueeController.js

+			this.cancelJob.stop();
+		}
+
+		cancelJob = new Job(() => this.doCancel(), 30)


30 seems like a magic number - what was the reasoning behind the specific value?

It is a bit magical. It was supposed to be long enough to make it async but not so long that the user would notice a lag between unhovering/unspotting and the marquee stopping. It should be long enough that as the move they pointer between the different parts of a controlled set that it wouldn't reset. I was thinking 100-300ms was probably a reasonable number but 30 seems to work well in testing. It may be possible to go without it given the ordering of the events but I didn't want to rely on possibly unpredictable event ordering.

🎩 🐰

Admittedly feels a bit dirty...

aarontam · 2017-11-15T22:24:15Z

packages/moonstone/LabeledItem/tests/LabeledItem-specs.js

@@ -44,7 +44,7 @@ describe('LabeledItem Specs', () => {
 	});

 	it('should have \'disabled\' HTML attribute when \'disabled=true\'', function () {
-		const item = mount(
+		const item = shallow(


Assuming we couldn't do a shallow test here, there should be a cleaner way to write this test so that we aren't checking the specific number of elements (1) with a disabled prop set to true.

cc @viodragon2

aarontam · 2017-11-15T22:35:03Z

packages/moonstone/Marquee/MarqueeController.js

-					onFocus: this.handleFocus
+					onFocus: this.handleFocus,
+					// When picker button becomes disabled, it doesn't fire blur, but does fire
+					// `onSpotlightDisappear`.  We should investigate why `onBlur` does not fire


Any clues from facebook/react#9142 (comment)?

Interesting find! Could be related. Talked some with @germboy about it but we didn't reach any conclusions.

aarontam · 2017-11-16T06:04:13Z

packages/moonstone/Marquee/MarqueeDecorator.js

-				return null;
-			});
+			if (!this.sync) {
+				this.setState((prevState) => {


Worth throwing this into a fn?

aarontam · 2017-11-16T06:06:06Z

packages/moonstone/internal/Picker/PickerButton.js

@@ -26,10 +28,35 @@ const PickerButtonBase = kind({
 		spotlightDisabled: PropTypes.bool
 	},

+	defaultProps: {
+		joined: false


Is this necessary?

No, not any longer since I removed forProp(). Just forgot to clean it up.

aarontam · 2017-11-16T06:18:01Z

packages/moonstone/Marquee/MarqueeDecorator.js

@@ -299,6 +299,8 @@ const MarqueeDecorator = hoc(defaultConfig, (config, Wrapped) => {
 				this.textDirectionValidated = false;
 			} else if (next.marqueeOn !== marqueeOn || next.marqueeDisabled !== marqueeDisabled || next.marqueeSpeed !== marqueeSpeed) {
 				this.cancelAnimation();
+			} else if (next.disabled && this.isHovered && marqueeOn === 'focus' && this.sync) {


Can you explain how the marqueeOn === 'focus' check helps in this scenario?

In this scenario, we're checking for a transition to disabled when we were in 'focus' start. In this case, we could get a 'blur' event, which would cancel the marquee. Since we're hovered, we want to make sure the marquee continues.

Not to mention that we don't fire the 'enter' event when focused and enabled.

aarontam · 2017-11-16T06:22:22Z

packages/moonstone/Marquee/MarqueeController.js

+			if (!this.anyRunning()) {
+				this.dispatch('start');
+			}
+			this.cancelJob.stop();


Do we need to stop the job if we're hovered now? Will the job's fn exit early?

🤔 We -could- rely on the job to run out and be trapped by the hover event. It seemed wiser to always clean up the job than to rely on logic elsewhere. I'm game for removing these extra ones.

aarontam · 2017-11-16T06:23:04Z

packages/moonstone/Marquee/MarqueeController.js

+			if (!this.anyRunning()) {
+				this.dispatch('start');
+			}
+			this.cancelJob.stop();


Similar question here as for handleEnter.

Similar answer here as for handleEnter.

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Roy Sutton and others added 13 commits November 9, 2017 18:26

Allow MarqueeController more control

a8b70cb

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Apply same to focus/blur

97db4d6

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Fix Spottable and PickerButton

a6e9b0a

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Stop job on unmount

43ba215

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Don't clobber handlers for joined picker

6770d65

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Make sure joined is boolean

2c4f9dc

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Fix leaving picker button after disable

eccc37c

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Trigger with disabled joined button

828998b

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Fix LabeledItem to marquee on disabled

237bf44

Allow disabled joined picker to marquee

b7264cd

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Fix linter warning

ca7eef6

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Allow non-joined Picker to handle onMouseEnter/Leave

67baa87

Fix test

7c3be74

viodragon2 approved these changes Nov 14, 2017

View reviewed changes

aarontam added 3 commits November 15, 2017 14:11

Merge branch 'release/1.x' into feature/ENYO-4811-roysutton

4a0b920

Fixes up change log entry positioning

7e966cf

Fixes up bad merge

963e3ba

aarontam reviewed Nov 15, 2017

View reviewed changes

aarontam reviewed Nov 16, 2017

View reviewed changes

Roy Sutton added 3 commits November 16, 2017 15:30

Extract common code and clean up

858a079

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Fix up CHANGELOG problems

6211278

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Fix up CHANGELOG problems

48c88f7

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

aarontam merged commit 9f96386 into release/1.x Nov 16, 2017

aarontam deleted the feature/ENYO-4811-roysutton branch November 16, 2017 21:44

teckliew mentioned this pull request Apr 10, 2018

ENYO-4811: Picker Marquee Fix #1239

Closed

ryanjduffy mentioned this pull request Sep 7, 2018

ENYO-5623: Fix ui/Marquee to stop when blurred during restart timer #1935

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/enyo 4811 roysutton #1301

Feature/enyo 4811 roysutton #1301

webOS101 commented Nov 10, 2017 •

edited

Loading

viodragon2 left a comment

aarontam left a comment

aarontam Nov 15, 2017

webOS101 Nov 15, 2017 •

edited

Loading

aarontam Nov 16, 2017 •

edited

Loading

aarontam Nov 15, 2017

aarontam Nov 16, 2017

aarontam Nov 15, 2017

webOS101 Nov 16, 2017

aarontam Nov 16, 2017

webOS101 Nov 16, 2017

aarontam Nov 16, 2017

webOS101 Nov 16, 2017

aarontam Nov 16, 2017

webOS101 Nov 16, 2017

webOS101 Nov 16, 2017

aarontam Nov 16, 2017

webOS101 Nov 16, 2017

aarontam Nov 16, 2017

webOS101 Nov 16, 2017

Feature/enyo 4811 roysutton #1301

Feature/enyo 4811 roysutton #1301

Conversation

webOS101 commented Nov 10, 2017 • edited Loading

Issue Resolved / Feature Added

Resolution

Additional Considerations

Links

Comments

viodragon2 left a comment

Choose a reason for hiding this comment

aarontam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webOS101 Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

aarontam Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webOS101 commented Nov 10, 2017 •

edited

Loading

webOS101 Nov 15, 2017 •

edited

Loading

aarontam Nov 16, 2017 •

edited

Loading