Added `keepAttachedTo` method to the BalloonPanelView. #172

oskarwrobel · 2017-03-20T18:34:16Z

Suggested merge commit message (convention)

Feature: Added keepAttachedTo() method to the BalloonPanelView. Closes ckeditor/ckeditor5#5320.

Follow-ups:

Balloon panel sticks out of the limiter element while scrolling ckeditor5#5328 - Panel sticks out of the container element while scroll

oskarwrobel · 2017-03-22T16:00:16Z

src/panel/balloon/balloonpanelview.js

+		this.attachTo( options );
+
+		// Then we need to listen to scroll of eny element in the document and update position of the balloon panel.
+		this.listenTo( document, 'scroll', () => this.attachTo( options ), { useCapture: true } );


The same should be for resize event.

Yes, that's right.

oleq · 2017-03-23T09:01:27Z

src/panel/balloon/balloonpanelview.js

+		this.attachTo( options );
+
+		// Then we need to listen to scroll of eny element in the document and update position of the balloon panel.
+		this.listenTo( document, 'scroll', () => this.attachTo( options ), { useCapture: true } );


This useCapture that you put in here makes the listener very generic. Because of that, events fired in the document will trigger the callback in the capture phase. And since the listener is attached to document, every single scroll event coming from whatever element with overflow: scroll will trigger it. Such element doesn't even need to have anything in common with CKEditor. document aggregates all events with useCapture because it's a root.

For one thing, it's OK. Because the balloon will re–position when elements with overflow: scroll will scroll. But still, it's too much. We need to filter out the events that have no impact on balloon panel, which are events comming from elements that are neither:

the ancestor of the ck-body (or wherever the balloon is located in DOM),

nor the ancestor of the target in attachTo().

Node.contains() will help a lot.

I limited updating position to scroll events from elements containing target and limiter elements.

oleq · 2017-03-23T09:03:41Z

tests/panel/balloon/balloonpanelview.js

@@ -409,6 +409,55 @@ describe( 'BalloonPanelView', () => {
 			} );
 		} );
 	} );
+
+	describe( 'stickTo()', () => {


We need a test to verify that use of useCapture makes sense and actually works.

We need a manual test for BalloonPanel because it's getting quite complex. I think we could develop it on top of http://localhost:8125/ckeditor5-ui/tests/manual/tickets/126/1.html.

I prepared dedicated test for this ticket: http://localhost:8125/ckeditor5-ui/tests/manual/tickets/170/1.html this test compares attachTo vs keepAttachedTo.

oleq · 2017-03-23T09:07:58Z

src/panel/balloon/balloonpanelview.js

+	 * {@link module:utils/dom/position~getOptimalPosition}. Default `positions` array is
+	 * {@link module:ui/panel/balloon/balloonpanelview~BalloonPanelView.defaultPositions}.
+	 */
+	stickTo( options ) {


I think we should reference the attachTo method in this name because it's essentially the same thing. Like keepAttachedTo albo attachPermanentlyTo.

This could even become a parameter of attachTo options. I wonder which naming/grouping strategy is the best /cc @Reinmar.

Renamed to keepAttachedTo.

oleq · 2017-03-31T10:32:03Z

src/panel/balloon/balloonpanelview.js

+		}, { useCapture: true } );
+
+		// We need to listen on window resize event and update position.
+		this.listenTo( window, 'resize', () => this.attachTo( options ) );


Please use https://github.com/ckeditor/ckeditor5-utils/blob/master/src/dom/global.js#L1-L26 instead. It helps a lot stubbing things when writing tests.

oleq · 2017-03-31T10:32:12Z

src/panel/balloon/balloonpanelview.js

-		this.once( 'change:isVisible', () => this.stopListening( document, 'scroll' ) );
+		this.once( 'change:isVisible', () => {
+			this.stopListening( document, 'scroll' );
+			this.stopListening( window, 'resize' );


Please use https://github.com/ckeditor/ckeditor5-utils/blob/master/src/dom/global.js#L1-L26 instead. It helps a lot stubbing things when writing tests.

oleq

Minor fixes needed.

The more important question is: does the new method get the work done for you @oskarwrobel?

Also: I wonder if method renaming would make sense: attachTo() => setPosition(), keepAttachedTo() => attachTo()?

oleq · 2017-04-03T12:47:35Z

src/panel/balloon/balloonpanelview.js

+		this.attachTo( options );
+
+		const target = options.target;
+		const limiter = options.limiter || defaultLimiterElement;


This line lacks test coverage.

oleq · 2017-04-03T12:48:32Z

tests/manual/tickets/170/1.html

+				<p></p>
+				<p></p>
+				<p></p>
+				<p>Balloon is attached to the <strong>TARGET</strong> element.</p>


Please use position: absolute or simply large margin and drop these paragraphs. It just looks weird :D

oleq · 2017-04-03T12:51:13Z

tests/manual/tickets/170/1.js

+import Template from '@ckeditor/ckeditor5-ui/src/template';
+
+// Content of the balloon panel.
+class BalloonContentView extends View {


KISS. Add simple styles

.ck-balloon-panel { padding: 2em; }

and then balloonPanel.innerHTML = 'Balloon'. It should be about what is important in that particular test case, not about balloon content, view nesting and the such. It's just a noise.

oleq · 2017-04-03T12:53:38Z

tests/panel/balloon/balloonpanelview.js

+			expect( attachToSpy.lastCall.args[ 0 ] ).to.deep.equal( { target, limiter } );
+		} );
+
+		it( 'should stop attach when balloon is hidden', () => {


I think this test could be merged with 'should stop attach when balloon is hidden'.

oleq · 2017-04-03T12:54:10Z

tests/panel/balloon/balloonpanelview.js

+			expect( attachToSpy.lastCall.args[ 0 ] ).to.deep.equal( { target, limiter } );
+		} );
+
+		it( 'should stop attach when balloon is hidden', () => {


s/attach/attaching,
s/balloon/the balloon,

oleq · 2017-04-03T12:55:40Z

src/panel/balloon/balloonpanelview.js

+	/**
+	 * Works exactly the same as {module:ui/panel/balloon/balloonpanelview~BalloonPanelView.attachTo} with one exception.
+	 * Position of attached panel is constantly updated when any of parents of the target or limiter elements are scrolled
+	 * or when browser window is resized. Thanks to this panel always sticks to the target element. See #170.


Full link to the issue would be safer. We had code moving between repos and such hashes became outdated more than once already.

oleq · 2017-04-03T14:13:59Z

Afterthought: Scroll/resize listeners should be killed on component destroy().

oskarwrobel · 2017-04-03T14:16:04Z

Also: I wonder if method renaming would make sense: attachTo() => setPosition(), keepAttachedTo() => attachTo()?

I was thinking about one attachTo method which should always update position on scroll and resize.

oleq · 2017-04-04T08:31:01Z

src/panel/balloon/balloonpanelview.js

+		// Then we need to listen on scroll event of eny element in the document.
+		this.listenTo( global.document, 'scroll', ( evt, domEvt ) => {
+			// And update position if scrolled element contains related to the balloon elements.
+			if ( domEvt.target.contains( target ) || domEvt.target.contains( limiter ) ) {


What would happen if options.target or options.limiter are Range or ClientRect? I suppose it's not gonna work and it may throw errors.

For the DOM Range we can do it by obtaining range ancestor Node. For ClientRect, there's nothing we can do because it's an abstract geometric entity, I guess. Right?

…om DOM "scroll" listener.

oleq · 2017-04-05T10:16:42Z

OK. It's R+ but since the master is locked we postpone the merge.

Added stickTo method to the BalloonPanelView.

2d9f309

oskarwrobel commented Mar 22, 2017

View reviewed changes

oskarwrobel requested a review from oleq March 22, 2017 16:00

oleq reviewed Mar 23, 2017

View reviewed changes

oleq added the status:review- label Mar 23, 2017

oskarwrobel added 2 commits March 31, 2017 10:34

Limited balloon panel updaing on scroll.

3a6b741

Updated panel position on resize.

9233160

oleq reviewed Mar 31, 2017

View reviewed changes

oskarwrobel added 3 commits March 31, 2017 13:18

Added manual test for BalloonPanelView#keepAttachedTo.

a5c1dab

Used global namespace for global dom object.

4d1443f

Moved manual test to tickets.

e078d48

oskarwrobel removed the status:review- label Mar 31, 2017

Merge branch 'master' into t/170

30904a7

oleq suggested changes Apr 3, 2017

View reviewed changes

oleq added the status:review- label Apr 3, 2017

oskarwrobel added 5 commits April 3, 2017 17:07

Added full url to the ticket in docs.

a3f0934

Simplified manual test.

fc6ec1a

Reduced similar test cases.

724a590

Cleaned view listeners in destroy method.

f3840ae

Added test for default limiter.

8692e48

oskarwrobel removed the status:review- label Apr 3, 2017

Docs: Enhanced BalloonPanelView#keepAttachedTo() docs.

3941c56

oleq reviewed Apr 4, 2017

View reviewed changes

oleq added the status:review- label Apr 4, 2017

oskarwrobel changed the title ~~Added stickTo method to the BalloonPanelView.~~ Added keepAttachedTo method to the BalloonPanelView. Apr 4, 2017

oskarwrobel added 4 commits April 4, 2017 16:09

Added support for Range and Rect as a target.

af93584

Using sinon assertions in tests.

4a31312

Added full ticket url.

dd62d00

Simplfied the test.

5fbbe27

oskarwrobel removed the status:review- label Apr 4, 2017

oleq and others added 3 commits April 5, 2017 11:45

Optimized BalloonPanelView#keepAttachedTo by extracting some logic fr…

b7d759e

…om DOM "scroll" listener.

Improved test.

e369e41

Fixed failing test.

3c55902

oleq added the status:review+ label Apr 5, 2017

oskarwrobel merged this pull request into master Apr 5, 2017

oskarwrobel deleted the t/170 branch April 5, 2017 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `keepAttachedTo` method to the BalloonPanelView. #172

Added `keepAttachedTo` method to the BalloonPanelView. #172

oskarwrobel commented Mar 20, 2017 •

edited by pomek

Loading

oskarwrobel Mar 22, 2017

oleq Mar 23, 2017

oleq Mar 23, 2017

oskarwrobel Mar 31, 2017

oleq Mar 23, 2017

oskarwrobel Mar 31, 2017 •

edited

Loading

oleq Mar 23, 2017

oskarwrobel Mar 31, 2017 •

edited

Loading

oleq Mar 31, 2017

oskarwrobel Mar 31, 2017

oleq Mar 31, 2017

oskarwrobel Mar 31, 2017

oleq left a comment •

edited

Loading

oleq Apr 3, 2017

oskarwrobel Apr 3, 2017

oleq Apr 3, 2017

oskarwrobel Apr 3, 2017

oleq Apr 3, 2017

oskarwrobel Apr 3, 2017

oleq Apr 3, 2017

oskarwrobel Apr 3, 2017

oleq Apr 3, 2017

oskarwrobel Apr 3, 2017

oleq Apr 3, 2017

oskarwrobel Apr 3, 2017

oleq commented Apr 3, 2017

oskarwrobel commented Apr 3, 2017

oleq Apr 4, 2017 •

edited

Loading

oskarwrobel Apr 4, 2017

oleq commented Apr 5, 2017

Added keepAttachedTo method to the BalloonPanelView. #172

Added keepAttachedTo method to the BalloonPanelView. #172

Conversation

oskarwrobel commented Mar 20, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Follow-ups:

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

oskarwrobel Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel Mar 31, 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

oleq left a comment • 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

oleq commented Apr 3, 2017

oskarwrobel commented Apr 3, 2017

oleq Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq commented Apr 5, 2017

Added `keepAttachedTo` method to the BalloonPanelView. #172

Added `keepAttachedTo` method to the BalloonPanelView. #172

oskarwrobel commented Mar 20, 2017 •

edited by pomek

Loading

oskarwrobel Mar 31, 2017 •

edited

Loading

oskarwrobel Mar 31, 2017 •

edited

Loading

oleq left a comment •

edited

Loading

oleq Apr 4, 2017 •

edited

Loading