Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented May 4, 2020

Related issues: flutter/gallery#143

@ferhatb ferhatb requested a review from yjbanov May 4, 2020 20:25
@ferhatb ferhatb requested a review from harryterkelsen May 4, 2020 20:26
final int poolLength = _pool.length;
canvasCount += poolLength;
for (int i = 0; i < poolLength; i++) {
_pool[i].style.zIndex = (-canvasCount + i).toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this change, but the fact that I can never remember what _pool and _reusabltPool contains probably means we could use better names for these variables 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Possible Blink bugs that are causing this:
// * https://bugs.chromium.org/p/chromium/issues/detail?id=370604
// * https://bugs.chromium.org/p/chromium/issues/detail?id=586601
int canvasCount = _canvas == null ? 0 : 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is canvasCount not straight _pool.length?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted

element.style.overflow = 'hidden';
element.style
..overflow = 'hidden'
..zIndex = '0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if it matters, but z-index: 0 is not the same as default z-index: https://www.w3.org/TR/CSS21/zindex.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, spec says it will be treated atomically which seems to help with failure rather than tree order. Since it has single child should not matter.

List<html.CanvasElement> list =
contentAfterReuse.querySelectorAll('canvas');
expect(list[0].style.zIndex, '-2');
expect(list[1].style.zIndex, '-1');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have two canvases, with z-index -2 and -1, that means that there's an image or text that's between them. Shouldn't the image/text have a z-index that's between the two canvas' z-indexes? The other issue is that if both a canvas and text have negative z-index, then the canvas will appear on top of text due to the Chrome/Safari bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted per offline discussion.

int _saveContextCount = 0;
int _activeCanvasCount = 0;
// Number of elements that have been added to flt-canvas.
int _activeElementCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not always have a non-null non-empty _pool? Then _canvas becomes _pool.last, and _activeElementCount becomes _pool.length, and we can remove all the null checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pool only tracks canvas(s) if you have more than 1 canvas. activeElementCount tracks both canvas, p and img tags. So if you draw image and then canvas, activeElementCount is 2 at the end.

@ferhatb
Copy link
Contributor Author

ferhatb commented May 5, 2020

Mac Web Engine retry passed in https://ci.chromium.org/p/flutter/builders/try/Mac%20Web%20Engine/3028

@ferhatb ferhatb merged commit 4b7380b into flutter:master May 5, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2020
@ferhatb ferhatb deleted the composite2 branch May 8, 2020 19:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants