301149 - :hover is processed in Print Preview mode

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Mats Palmgren (inactive)]

:hover is processed in Print Preview mode

STEPS TO REPRODUCE
1. load attached testcase
2. File -> Print Preview
3. move mouse over elements

ACTUAL RESULTS
DIV and IFRAME disappears when hovered, INPUT doesn't.

EXPECTED RESULTS
Nothing disappears (:hover not processed).

BUILDS & PLATFORMS TESTED
Bug occurs in SeaMonkey 2005-07-11-02 trunk Linux

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Boris Zbarsky [:bzbarsky]]

The basic problem here is that print preview just looks like a random screen
presentation in most ways.... :(

Comment 3

[zug_treno]

Related to/duplicate of bug 143471?

Comment 4

[Julian]

Just like to add that this web address causes the problem as well.

http://www.lightreading.com/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this bug is not INVA. Because the print preview mode is a simulator of the result of the printing. By the limit of the device, the result of the printing doesn't have :hover effect. Therefore, the print preview mode should simulate the same limit.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

This fixes this issue.

I think that the focus state and the active state are ignored in print preview mode. If this bug is INVA. We should support those state in print preview.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0.1

Oops, the previous patch has wrong indent.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Doesn't that patch break hover on print-preview scrollbars (when those have a hover effect)?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 241872 [details] [diff] [review]
Patch rv1.0.1

> Doesn't that patch break hover on print-preview scrollbars (when those have a
> hover effect)?

You're right.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

This is checking whether the content is in a scrollbar element of XUL.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 241942 [details] [diff] [review]
Patch rv2.0

I think that we should fix this issue, so I still think that this is a bug.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I don't think I should be reviewing this code.  I certainly won't have time to any time in the foreseeable future.

Comment 13

[Martijn Wargers (dead)]

So with patch rv2.0, scrollbars in content on print preview would still get a hover effect?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 241942 [details] [diff] [review]
Patch rv2.0

(In reply to comment #12)
> I don't think I should be reviewing this code.  I certainly won't have time to
> any time in the foreseeable future.

O.K. changing to Roc.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #13)
> So with patch rv2.0, scrollbars in content on print preview would still get a
> hover effect?

There are no scrollbars in the content of print preview on WinXP.
On other platforms, do the scrollbars appear in the content of print preview?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.0

Oops, sorry. The previous patch doesn't cancel the :hover state of the scrollbar.
This patch works fine. And this checking whether the scrollbar owner is root content.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 241953 [details] [diff] [review]
Patch rv3.0

Call !mPresContext->IsDynamic() instead of checking the type.

This would allow XUL scrollbars that are children of the root element to be hovered even if they're not the real viewport scrollbars.

You also should really check for the scrollcorner. But this is fragile because it will break if we change scrollframe anonymous content.

A cleaner way to solve this would be to call GetPrimaryFrameFor(aContent), Get the root scrollframe via nsIPresShell::GetRootScrollFrame, QI it to nsIScrollableFrame, call GetScrolledFrame, and check whether aContent's frame IS a descendant of the scrollableFrame but IS NOT a descendant of the scrolledFrame. You might want to put this check in nsLayoutUtils because I have a feeling that we've implemented it elsewhere...

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv4.0

Thank you, Roc! It's clever idea!

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

*** Bug 143471 has been marked as a duplicate of this bug. ***

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (!aContent || !aPresContext || !aContent->IsNodeOfType(nsINode::eXUL))
+    return PR_FALSE;

Skip the IsNodeOfType check. It's not needed, and it might hurt if we ever have nonXUL elements in scroll widgets (someone could actually do this with XBL)

+  nsIPresShell* presShell = aPresContext->GetPresShell();
+  if (!presShell)
+    return PR_FALSE;

Just call ->PresShell(), no need to null check.

+  nsCOMPtr<nsIScrollableFrame>
+    rootScrollableFrame = do_QueryInterface(rootScrollFrame);
+  if (!rootScrollableFrame)
+    return PR_FALSE;

Make this null-check an assertion. It should never fail if GetRootScrollFrame is working correctly.

+  if (!rootScrolledFrame || !IsProperAncestorFrame(rootScrolledFrame, frame))
+    return PR_TRUE;
+
+  return PR_FALSE;

Just "return !rootScrolledFrame || !IsProperAncestorFrame(rootScrolledFrame, frame);"

Actually I think it would be slightly nicer to make IsViewportScrollbarContent be IsViewportScrollbarFrame and take a single nsIFrame* parameter. (If we ever want to use it again we're more likely to have a frame than an nsIContent*). You can then write
  nsIFrame* rootScrollFrame = aFrame->GetPresContext()->PresShell()->GetRootScrollFrame();
and continue on as before. The GetPrimaryFrameFor would be moved to SetContentState.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv5.0

Sorry for the delay.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Roc:

Would you continue to review?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I lost this somehow.

+PRBool
+nsLayoutUtils::IsViewportScrollbarFrame(nsPresContext* aPresContext,
+                                        nsIFrame* aFrame)

Don't pass aPresContext as a parameter. Just use aFrame->GetPresContext() (which doesn't need to be null-checked).

+  nsCOMPtr<nsIScrollableFrame>
+    rootScrollableFrame = do_QueryInterface(rootScrollFrame);

Use
  nsIScrollableFrame* rootScrollableFrame;
  CallQueryInterface(rootScrollFrame, &rootScrollableFrame);
since frames aren't reference-counted.

+      } else if (aState == NS_EVENT_STATE_HOVER && !mHoverContent) {
+        // We need to do nothing, because any content is not changed the state.
+        return PR_FALSE;

Do we need this case? It seems to me we could just do newHover = nsnull; and carry on, and it would be correct. If so, then I think we should remove this case to simplify the code.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv6.0

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 244275 [details] [diff] [review]
Patch rv6.0

IsViewportScrollbarFrame should return false for the root scrolled frame. Currently it returns true. So

+  return !rootScrolledFrame ||
+         !IsProperAncestorFrame(rootScrolledFrame, aFrame);

Make this !(rootScrolledFrame == aFrame || IsProperAncestorFrame(rootScrollFrame, aFrame))

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 244275 [details] [diff] [review]
Patch rv6.0

IsViewportScrollbarFrame should return false for the root scrolled frame. Currently it returns true. So

+  return !rootScrolledFrame ||
+         !IsProperAncestorFrame(rootScrolledFrame, aFrame);

Make this !(rootScrolledFrame == aFrame || IsProperAncestorFrame(rootScrollFrame, aFrame))

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv6.1

Thank you, roc.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in, thank you.