904328 - make coordinates of display items relative to scroll root

Status: NEW

(Core :: Layout, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Thinker Li [:sinker]]

Comment on attachment 789316 [details] [diff] [review]
patch

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

::: layout/generic/nsQueryFrame.h
@@ +93,5 @@
> +  }
> +
> +  template<class Dest>
> +  operator Dest*() {
> +    return (Dest *) reinterpret_cast<const Dest *>(this);

This line has two problems.
1. reinterpret_cast does not call casting operator.  So, first operator Dest*() would not be called.  static_cast should be used to replace reinterpret_cast.
2. operator Dest*() is applied on type do_|QueryFrame|, not on type |(do_QueryFrame *)|, so |*this| should be used instead of |this|.

This line is too fancy and too elaborate to be understood XD

Comment 3

[Thinker Li [:sinker]]

Attachment: Modified version of Andreas's patch

Fix |operator new| function of |do_QueryFrame| and translating linear spaces of |gfxContext|s for |nsDisplayItem|s.

Comment 4

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #3)
> Created attachment 799304 [details] [diff] [review]
> Modified version of Andreas's patch
> 
> Fix |operator new| function of |do_QueryFrame| and translating linear spaces
> of |gfxContext|s for |nsDisplayItem|s.

Native widgets are not draw correctly.

Comment 5

[Thinker Li [:sinker]]

Attachment: Modified version of Andreas's patch, v2

Fix the issue of drawing popup menu (native widget I had mentioned).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 801240 [details] [diff] [review]
Modified version of Andreas's patch, v2

Could you explain what feedback you wanted?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 801240 [details] [diff] [review]
Modified version of Andreas's patch, v2

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3156,5 @@
>  
>    FrameLayerBuilder *layerBuilder = aLayer->Manager()->GetLayerBuilder();
>    NS_ASSERTION(layerBuilder, "Unexpectedly null layer builder!");
>  
> +  nsIFrame *rootReferenceFrame = builder->RootReferenceFrame();

Most of your *s and &s are on the wrong side; should be next to the type, not the variable.

Comment 8

[Thinker Li [:sinker]]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #6)
> Could you explain what feedback you wanted?

Do you think the idea here is reasonable?  I mean to use a reference frame without a view.  It makes an exception of root referenced frame.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Let's ask Matt.

Comment 10

[Matt Woodrow (:mattwoodrow)]

As I said on the mailing list, I think we want to drop the translation code in FrameLayerBuilder, and instead make sure that we always get an nsDisplayScrollLayer (and an actual ContainerLayer) for this reference frame, and apply the necessary translation to the ContainerLayer.

Comment 11

[Thinker Li [:sinker]]

Attachment: Modified version of Andreas's patch, v3

Fix a minor bug in FindReferenceFrameFor().