428037 - Need ability to reftest "print-selection" feature

Status: NEW

(Testing :: Reftest, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: example testcase with automatically-selected text

In working on Bug 402264, I've decided that we really need to be able to reftest the print-selection feature, to make sure it doesn't regress again.  It shouldn't actually be that hard.

I'm attaching a testcase that demonstrates how to select a node at pageload time, and after that, it's just a matter of doing something like "reftest-print" but with with printSettings->GetPrintRange returning nsIPrintSettings::kRangeSelection.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> It shouldn't actually be that hard.
> ...
> it's just a matter of doing something like
> "reftest-print" but with with printSettings->GetPrintRange returning
> nsIPrintSettings::kRangeSelection.

Actually, it may be more complicated than I'd thought...

I just GDB'd through a 'reftest-print' run -- it looks like that process never calls nsPrintEngine::ReflowPrintObject, which is one of the main places where we handle print-selection.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch wip v1

Here's a simple WIP patch, which:
 - tweaks the "reftest-print" section of reftest.js
 - creates a subdir for print-selection reftests, with a simple one to start with

Comment 3

[Daniel Holbert [:dholbert]]

(though the patch is WIP because it still doesn't address the issue that we're never calling nsPrintEngine::ReflowPrintObject, mentioned in comment 1)

Comment 4

[Daniel Holbert [:dholbert]]

Requesting wanted1.9.1 -- this is crucial for preventing print-selection regressions, and the sooner we can start automated testing for those, the better.

Comment 5

[Eli Friedman]

Fixing this essentially requires moving the computation of the selection bounds (currently in nsPrintEngine) into nsSimplePageSequenceFrame::Reflow, and moving the multi-page print selection paint logic (currently in nsSimplePageSequenceFrame::PrintNextPage) into nsSimplePageSequenceFrame::PaintPageSequence.

The risk for these changes is relatively high, although the changes won't affect anything outside of printing.

Comment 6

[:Hb]

Bug 428111 and bug 431588 want to print only the selected parts of an element. 

The example attachment 314623 [details] selects the whole DIV and would miss these cases.

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix+test

This adds a "reftest-print-selection" class.  It's like "reftest-print"
but only paints the selection.  Most of the layout code is already in
place for this (for Print - Selection), we just need to propagate the
print setting to the pres context.  There were a couple of snafus:
1. the page frame in Print Preview has a box shadow, we need to paint
   that even though the page frame isn't part of the selection
   (IsVisibleForPainting is false), so we need an unconditional version
   of DisplayBorderBackgroundOutline.
2. since selection is disabled in Print Preview, the text was painted
   in "disabled" grey color.  GetSelectionTextColors should return
   false to paint the text in normal colors.

(DisplayBorderBackgroundOutlineUnconditional is a mouthful, but it
looks like that's the naming convention for similar methods)

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=7090d18774c8

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 609068 [details] [diff] [review]
fix+test

>diff --git a/layout/generic/nsFrame.h b/layout/generic/nsFrame.h
>   nsresult DisplayBorderBackgroundOutline(nsDisplayListBuilder*   aBuilder,
>                                           const nsDisplayListSet& aLists,
>-                                          bool aForceBackground = false);
>+                                          bool aForceBackground = false) {
>+    // The visibility check belongs here since child elements have the
>+    // opportunity to override the visibility property and display even if
>+    // their parent is hidden.
>+    if (!IsVisibleForPainting(aBuilder))
>+      return NS_OK;
>+    return DisplayBorderBackgroundOutlineUnconditional(aBuilder, aLists, aForceBackground);

bump "aForceBackground" down to its own line, to stay under 80char

>--- a/layout/generic/nsTextFrameThebes.cpp
>+++ b/layout/generic/nsTextFrameThebes.cpp
>@@ -4862,17 +4862,19 @@ static void DrawSelectionDecorations(gfx
>  */
> static bool GetSelectionTextColors(SelectionType aType,
>                                      nsTextPaintStyle& aTextPaintStyle,
>                                      const nsTextRangeStyle &aRangeStyle,
>                                      nscolor* aForeground, nscolor* aBackground)

Maybe fix the indentation of 2nd,3rd,4th there ^ while you're touching this chunk?

> {
>   switch (aType) {
>     case nsISelectionController::SELECTION_NORMAL:
>-      return aTextPaintStyle.GetSelectionColors(aForeground, aBackground);
>+      return NS_UNLIKELY(aTextPaintStyle.PresContext()
>+                           ->IsRenderingOnlySelection()) ? false : 

Remove end-of-line space char after ":"

>+++ b/layout/tools/reftest/reftest-content.js
>+const PRINTMODE_NONE = 0;
>+const PRINTMODE_DOC = 1;
>+const PRINTMODE_SELECTION = 2;
>+
> function doPrintMode(contentRootElement) {
>     // use getAttribute because className works differently in HTML and SVG
>-    return contentRootElement &&
>-           contentRootElement.hasAttribute('class') &&
>-           contentRootElement.getAttribute('class').split(/\s+/)
>-                             .indexOf("reftest-print") != -1;
>+    if (contentRootElement &&
>+        contentRootElement.hasAttribute('class')) {
>+        if (contentRootElement.getAttribute('class').split(/\s+/)
>+            .indexOf("reftest-print") != -1)
>+            return PRINTMODE_DOC;
>+        if (contentRootElement.getAttribute('class').split(/\s+/)
>+            .indexOf("reftest-print-selection") != -1)
>+            return PRINTMODE_SELECTION;
>+    }
>+    return PRINTMODE_NONE;
> }

Could we call this "getPrintMode" or something now?  "doPrintMode" is a bit of an odd name for a function that returns an enum.

Also: what should the behavior be if someone specifies both "reftest-print" and "reftest-print-selection" simultaneously?  I'd expect either some sort of error to happen, or for "selection" to win (as a subset of "print") -- but it looks like right now the selection will be ignored.

>@@ -436,19 +449,22 @@ function WaitForTestEnd(contentRootEleme
>-            if (!inPrintMode && doPrintMode(contentRootElement)) {
>-                LogInfo("MakeProgress: setting up print mode");
>-                setupPrintMode();
>+            if (!inPrintMode) {
>+                var printMode = doPrintMode(contentRootElement);
>+                if (printMode != PRINTMODE_NONE) {

This check doesn't make sense to me -- I think "inPrintMode" is already a proxy for |printMode != PRINTMODE_NONE|, right?  (It looks like it in the removed code.)

Perhaps you instead want to entirely replace "inPrintMode" with a new enum-valued variable?

>@@ -518,44 +534,45 @@ function OnDocumentLoad(event)
>         if (shouldWaitForExplicitPaintWaiters() ||
>-            (!inPrintMode && doPrintMode(contentRootElement)) ||
>+            (!inPrintMode && doPrintMode(contentRootElement) != PRINTMODE_NONE) ||

As above -- it doesn't make sense to me that inPrintMode would be out-of-sync with what doPrintMode tells us... (just based on variable-naming at least)  If inPrintMode can become an enum (as suggested above), I'd imagine this would be simpler.

Comment 9

[Daniel Holbert [:dholbert]]

(Also: it's awesome that your fixing this! I'm very much looking forward to being able to regression-test print selection bugs. :))

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix+test, v2

Thanks, I fixed the nits as you suggested, except this one:

> "inPrintMode" is already a proxy for |printMode != PRINTMODE_NONE|, right?

No, inPrintMode is currently set only once, in OnDocumentLoad.
I believe the intention is to record if we have started "print mode" 
(ie, called docShell.contentViewer.setPageMode).
The inPrintMode you were referring to is in the WaitForTestEnd function
where inPrintMode is a parameter, so the code is rather subtle and hard
to understand.

I made it global, gInPrintMode, and now assign it in setupPrintMode()
to make the intention clearer.  So to clarify this block:

    if (!gInPrintMode) {
        var printMode = getPrintMode(contentRootElement);
        if (printMode != PRINTMODE_NONE) {
            LogInfo("MakeProgress: setting up print mode " + printMode);
            setupPrintMode(printMode);
        }
    }

If we're not in print mode yet, and we now have a print class, then
enter print mode.  Once we're in print mode, we stay there, and any
further changes to print classes will be ignored.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 609685 [details] [diff] [review]
fix+test, v2

Looks good to me!  Thanks for the clarification.

I don't think I've reviewed changes to the reftest harness before -- dbaron should probably sign off on this, too.

Comment 12

[Mats Palmgren (inactive)]

dbaron, review ping?

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 609685 [details] [diff] [review]
fix+test, v2

roc, can you rubber-stamp this? dholbert has already reviewed the code so
this is more of a sanity check of the approach than anything else.  Thanks.

Comment 14

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

Comment on attachment 609685 [details] [diff] [review]
fix+test, v2

>+    mPresContext->SetIsRenderingOnlySelection(printSelection);

Does this ever need to be undone?

>+    var printMode = getPrintMode(contentRootElement);
>+    if (printMode != PRINTMODE_NONE) {
>+        LogInfo("OnDocumentLoad setting up print mode " + printMode);
>+        setupPrintMode(printMode);
>+    }
>+
>     if (shouldWaitForReftestWaitRemoval(contentRootElement) ||
>         shouldWaitForExplicitPaintWaiters()) {
>         // Go into reftest-wait mode immediately after painting has been
>         // unsuppressed, after the onload event has finished dispatching.
>         gFailureReason = "timed out waiting for test to complete (trying to get into WaitForTestEnd)";
>         LogInfo("OnDocumentLoad triggering WaitForTestEnd");
>-        setTimeout(function () { WaitForTestEnd(contentRootElement, inPrintMode); }, 0);
>+        setTimeout(function () { WaitForTestEnd(contentRootElement); }, 0);
>     } else {
>-        if (doPrintMode(contentRootElement)) {
>-            LogInfo("OnDocumentLoad setting up print mode");
>-            setupPrintMode();
>-            inPrintMode = true;
>-        }
>-
>         // Since we can't use a bubbling-phase load listener from chrome,
>         // this is a capturing phase listener.  So do setTimeout twice, the
>         // first to get us after the onload has fired in the content, and
>         // the second to get us after any setTimeout(foo, 0) in the content.
>         gFailureReason = "timed out waiting for test to complete (waiting for onload scripts to complete)";
>         LogInfo("OnDocumentLoad triggering AfterOnLoadScripts");
>         setTimeout(function () { setTimeout(AfterOnLoadScripts, 0); }, 0);
>     }

Doesn't this break the interaction of reftest-wait and reftest-print?

Comment 15

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

Other than that, this looks fine.  Sorry for the delay.

Comment 16

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

Comment on attachment 609685 [details] [diff] [review]
fix+test, v2

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

::: layout/generic/nsTextFrameThebes.cpp
@@ +4869,5 @@
>  {
>    switch (aType) {
>      case nsISelectionController::SELECTION_NORMAL:
> +      return NS_UNLIKELY(aTextPaintStyle.PresContext()
> +                           ->IsRenderingOnlySelection()) ? false :

!IsRenderingOnlySelection() && GetSelectionColors()

Comment 17

[Mats Palmgren (inactive)]

Attachment: fix+test, v3

(In reply to David Baron [:dbaron] from comment #14)
> >+    mPresContext->SetIsRenderingOnlySelection(printSelection);
> 
> Does this ever need to be undone?

SetPageMode always creates a fresh PresContext/Shell:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#4304
I pretty sure we create a new one again when we leave page mode.
Anyway, if the IsRenderingOnlySelection bit were lingering after
leaving page mode, I'm sure it would break just about every test
after that.

> Doesn't this break the interaction of reftest-wait and reftest-print?

On the contrary, it fixes it.  With the current code, we send the
MozReftestInvalidate event before setting up page mode - this means that
any state associated with the context/shell, such as the Selection, that
the event handler prepared for the test is lost when we enter page mode
(see above).  We could solve that with a special event after page mode
has been setup, or a second MozReftestInvalidate, but that seems to just
complicate things since presumably it's page mode that you want to test
so we should just enter it immediately and have the first and only
MozReftestInvalidate be in page mode, with the right pres context/shell.
I see it as an advantage that tests for Page/Galley mode are processed
the same way by the reftest framework.

Still, I added a "gInPrintMode = false;" in there just in case some future
print test wants to do <html class="reftest-wait"> and later in its handler
do "document.documentElement.className='reftest-print'", to trigger the
setupPrintMode() in WaitForTestEnd().  I think we should remove this
path though to simplify the code.


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> !IsRenderingOnlySelection() && GetSelectionColors()

Fixed.

Comment 18

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

(In reply to Mats Palmgren [:mats] from comment #17)
> On the contrary, it fixes it.  With the current code, we send the
> MozReftestInvalidate event before setting up page mode - this means that
> any state associated with the context/shell, such as the Selection, that
> the event handler prepared for the test is lost when we enter page mode
> (see above).  We could solve that with a special event after page mode
> has been setup, or a second MozReftestInvalidate, but that seems to just
> complicate things since presumably it's page mode that you want to test
> so we should just enter it immediately and have the first and only
> MozReftestInvalidate be in page mode, with the right pres context/shell.
> I see it as an advantage that tests for Page/Galley mode are processed
> the same way by the reftest framework.

But we don't run script in page mode, do we?  So I thought the idea was that if you used reftest-wait *and* page mode, we wait to set up page mode until reftest-wait is removed.

Comment 19

[Mats Palmgren (inactive)]

> But we don't run script in page mode, do we?

The patch passed on Try so I don't see how that can be true.
Maybe you're thinking of real Print / Print Preview?
(which I believe clone the document, turns off script etc)

Comment 20

[Mats Palmgren (inactive)]

dbaron, do you have further comments/questions?

Comment 21

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

No, but I still tend to think that the combination of reftest-print and reftest-wait should switch to print mode when reftest-wait is removed.

Comment 22

[Mats Palmgren (inactive)]

The purpose of a reftest-print* test is to test paged layout and
MozReftestInvalidate is for preparing the test for the reftest snapshot.
Doing the MozReftestInvalidate in an environment (pres. shell/context)
that isn't the one used for the test makes no sense to me.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #21)
> No, but I still tend to think that the combination of reftest-print and
> reftest-wait should switch to print mode when reftest-wait is removed.

(In reply to Mats Palmgren [:mats] from comment #22)
> Doing the MozReftestInvalidate in an environment (pres. shell/context)
> that isn't the one used for the test makes no sense to me.

This makes at least some sense to me -- it matches how we actually do print / print-preview for an actual user.  We perform any dynamic changes in the unpaginated environment that the user sees when they load the page, and then when they tell us to print[-preview], we clone the document and paginate it, without allowing any further scripting in the clone.  This is roughly analogous to a reftest doing its dynamic tweaks up until reftest-wait is removed, and then switching to paginated mode at that point.

Comment 24

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

(In reply to Mats Palmgren [:mats] from comment #22)
> The purpose of a reftest-print* test is to test paged layout and
> MozReftestInvalidate is for preparing the test for the reftest snapshot.
> Doing the MozReftestInvalidate in an environment (pres. shell/context)
> that isn't the one used for the test makes no sense to me.

I don't see what this has to do with MozReftestInvalidate.  It's about reftest-wait.

Comment 25

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

OK, I changed my mind about the interaction of reftest-wait and reftest-print in bug 791480 -- on the basis that even though this is making the testing codepath less like the real codepath, there's really no point in doing dynamic modifications with the wrong presentation.  So roc is changing that aspect there.  Maybe you can pick this patch up again?

Comment 26

[Mats Palmgren (inactive)]

I'm probably not going to have time for this anytime soon.

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 619461 [details] [diff] [review]
fix+test, v3

Unhiding the last patch in case anyone else wants to pick it up.