Closed Bug 1338095 Opened 7 years ago Closed 7 years ago

[Mortar] Implement "View bookmark" button in PDF viewer.

Categories

(Firefox :: PDF Viewer, defect)

Product:
Firefox
Component:
PDF Viewer
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rexboy, Assigned: rexboy)

References

Details

Attachments

(1 file)

Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,
59 bytes, text/x-review-board-request
jj.evelyn
: review+
Details 
There's a feature called "view bookmark" which brings the current viewport status (like page position, zoom percentage, etc.) to the URL hash so that users can restore their viewport status by using the URL.
This bug is for studying and implementing this feature.


By the way when I click the button on PDF.js it looks like nothing happened if I didn't notice the URL change. Maybe some UI improvement on this situation(like showing a prompt message) is a plus.
Assignee: nobody → rexboy
See Also: → 1338094
See Also: 1338094 → 
Seems the coordinate system we used in Mortar is different from PDF.js.
One of the characteristics in PDF.js is the coordinate used is independent from zooming; whereas our scrolling position depends on zooming.
I'm not sure if preserving back compatibility with PDF.js is important. If yes we may need some extra effort to study about coordinate converting.
Update: The coordinate used in PDF.js is a standardized page coordinate, with the following characteristics:
1. it has its origin point at left bottom corner of *each* page.
2. 1 unit for 1/72 inches.
3. When we rotate the view, the axis also rotates. (Since it's page coordinate rather than screen coordinate.)

e.g. page=2&zoom=100,72,0 means the point 1 inch right against the left-bottom corner in page 2, with zoom 100%.

PDFium do have the coordinate converter but I'm not sure if it's available for external use. If it's not we may need to make a coordinate converter by ourselves or borrow it from PDF.js (if possible).
I spent some time on getting/setting hash since they needs passing messages from runtime and sandbox side.
Hopefully I can have a patch tomorrow.
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

This is a workable patch but I just tested with a few documents.

I think it's a good time for asking feedback about the patch since it changed not only UI but runtime and sandbox for changing URL.
Louis, Luke, and Evelyn may you take a look?

Some issues from my point of view:
- I'm not quite sure if the coordinate used in PDF.js is always scaled by 0.75 with respect to PDFium. If we don't care very much about the precise position on the page then this might not be in a first priority for digging further. Maybe testing it on different screens helps to clarify that.
- Generating hash after pressing the view bookmark button causes the view refresh again which is unnecessary, but that doesn't matters much anyway. If we care about that we may have a flag variable when sending out to prevent that.
Attachment #8840399 - Flags: feedback?(lochang)
Attachment #8840399 - Flags: feedback?(lchang)
Attachment #8840399 - Flags: feedback?(ehung)
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

Runtime and sandbox parts LGTM. But there is a wrong behavior when opening multiple pdf web pages that might due to multiple instances issue. Peter may have a local patch for that.

To reproduce the problem: first open a pdf page, then open another pdf page and generate a view bookmark. Paste the url with hash tag to first web page. And the first web page number will be wrong and it won't jump to the correct page number corresponding to the hash.
Attachment #8840399 - Flags: feedback?(lochang)
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

https://reviewboard.mozilla.org/r/114924/#review116618

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm:1556
(Diff revision 1)
> +      this.notifyHashChange(evt.data.url);
> +    });
> +  }
> +
> +  notifyHashChange(url) {
> +    let hash = url.split('#')[1];

Not sure if it's your intention, but splitting by `#` causes hash be truncated if there are two `#`s.

Though I don't think it's a problem since the string after the second `#` is unnecessary.

::: browser/extensions/mortar/host/pdf/chrome/js/viewer.js
(Diff revision 1)
>    let passwordPrompt = new PasswordPrompt(viewport);
>  
>    // Expose the custom viewport object to runtime
>    window.createCustomViewport = function(actionHandler) {
>      viewport.registerActionHandler(actionHandler);
> -

Looks like `viewer.js` has no other modifications. How about leaving it as-is.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:500
(Diff revision 1)
> +    // we keep it here for backward compability.
> +    let pageOrigin = {
> +      x: pageDimension.x,
> +      y: pageDimension.y + pageDimension.height
> +    };
> +    currentPos.x = currentPos.x / this._zoom;

nit: use `/=` shorthand to align it with others in the codebase (including yours below).

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:502
(Diff revision 1)
> +      x: pageDimension.x,
> +      y: pageDimension.y + pageDimension.height
> +    };
> +    currentPos.x = currentPos.x / this._zoom;
> +    currentPos.y = currentPos.y / this._zoom;
> +    currentPos.x = Math.round((currentPos.x - pageOrigin.x) * 0.75);

Since currentPos will have a different meaning (origin) after this line, I would prefer not reusing the same variable. How about storing new coordinate with new variables like `pageX` and `pageY`, which are the same as that in `_getScreenCooridnate`.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:532
(Diff revision 1)
> +  }
> +
> +  /**
> +   * @param hash
> +   *        contains page and zoom parameters which should be in the following
> +   *        format: page={page}&zoom={scale},{x},{y}

Is it case sensitive?

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:544
(Diff revision 1)
> +      this._initViewBookmark = hash;
> +      return;
> +    }
> +    this._initViewBookmark = null;
> +
> +

remove one of these two blank lines.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:550
(Diff revision 1)
> +    let params = hash.split('&');
> +    let pageNo, scale, pageX, pageY;
> +    params.forEach(param => {
> +      let [name, value] = param.split('=');
> +      if (name == 'page') {
> +        pageNo = parseInt(value);

Use `parseInt(value, 10)` If you don't want "0x123" be treated as a hexadecimal number. Otherwise, I'm fine with it now.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:559
(Diff revision 1)
> +        pageY = parseInt(pageY);
> +      }
> +    });
> +
> +    if (Number.isInteger(pageNo)) {
> +      pageNo = Math.max(0, Math.min(this.pageCount - 1, --pageNo));

Using `--pageNo` causes it to be unnecessarily assigned twice and makes it ambiguous. Try using `pageNo - 1` or put it in the previous line.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:567
(Diff revision 1)
> +    if (scale) {
> +      if (this._isValidFitting(scale)) {
> +        this._fitting = scale;
> +      } else {
> +        this._fitting = 'none';
> +        this._zoom = parseInt(scale) / 100;

`_zoom` will become 0 if `scale` isn't a valid fitting nor a number. Can we avoid that?

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:572
(Diff revision 1)
> +        this._zoom = parseInt(scale) / 100;
> +      }
> +    }
> +
> +    if (Number.isInteger(pageX) && Number.isInteger(pageY)) {
> +      let screenPos = this._getScreenCooridnate(pageNo, pageX, pageY);

According to this rule, a user will see the next page if `pageY` is omitted as it's treated as 0. After tring PDF.js' demo page, I think it treats the omitted `pageY` as the height of the current page so a user can see the beginning of the page specified by `page` argument. Could you confirm what rule PDF.js is using for this? Thanks.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:642
(Diff revision 1)
> +    let pagePosition = this._getPageCoordinate();
> +    let scale = this._fitting == 'none' ?
> +                  Math.round(this._zoom * 100) :
> +                  this._fitting;
> +    let hash = "page=" + (this._page + 1) +
> +           "&zoom=" + scale +

nit: it's not clear where you want these lines be indented. I would suggest aligning with the first `"` of `"page="` or two more spaces after that.

::: browser/extensions/mortar/host/pdf/chrome/viewer.html:138
(Diff revision 1)
>                  </button>
>  
>                  <button id="download" class="toolbarButton download hiddenMediumView" title="Download" tabindex="34" data-l10n-id="download">
>                    <span data-l10n-id="download_label">Download</span>
>                  </button>
> -                <a href="#" id="viewBookmark" class="toolbarButton bookmark hiddenSmallView" title="Current view (copy or open in new window)" tabindex="35" data-l10n-id="bookmark">
> +                <button id="viewBookmark" class="toolbarButton bookmark hiddenMediumView" title="Current view (copy or open in new window)" tabindex="35" data-l10n-id="bookmark">

Seems PDF.js used `<a>` here for changing hash without JS. Not sure if it benefits us. Could you take a look if we can get rid of `setHash` event? If not, I'm fine with `<button>`.
Attachment #8840399 - Flags: feedback?(lchang)
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

https://reviewboard.mozilla.org/r/114924/#review117732

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm:1556
(Diff revision 1)
> +      this.notifyHashChange(evt.data.url);
> +    });
> +  }
> +
> +  notifyHashChange(url) {
> +    let hash = url.split('#')[1];

Yeah I didn't handle that case because it's an illegal input anyway. The sanitizing process can be tested and strenghtened in my next version.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:550
(Diff revision 1)
> +    let params = hash.split('&');
> +    let pageNo, scale, pageX, pageY;
> +    params.forEach(param => {
> +      let [name, value] = param.split('=');
> +      if (name == 'page') {
> +        pageNo = parseInt(value);

We don't have such type of input from PDF.js anyway. We can decide whether to allow that or just refuse that.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:567
(Diff revision 1)
> +    if (scale) {
> +      if (this._isValidFitting(scale)) {
> +        this._fitting = scale;
> +      } else {
> +        this._fitting = 'none';
> +        this._zoom = parseInt(scale) / 100;

Sure I can try to give it a default value.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:572
(Diff revision 1)
> +        this._zoom = parseInt(scale) / 100;
> +      }
> +    }
> +
> +    if (Number.isInteger(pageX) && Number.isInteger(pageY)) {
> +      let screenPos = this._getScreenCooridnate(pageNo, pageX, pageY);

For now it needs both pageX and pageY are present to be recognized as vaild. But yeah I can test what PDF.js do for just one coordinate.
That would be a very rare case which may caused by user hand-typing since PDF.js always generates two coordinates together.
That's strange for MozReview. Let me do it again on Bugzilla.

(In reply to Luke Chang [:lchang] from comment #7)
> > +    let hash = url.split('#')[1];
> 
> Not sure if it's your intention, but splitting by `#` causes hash be
> truncated if there are two `#`s.
> 
> Though I don't think it's a problem since the string after the second `#` is
> unnecessary.

Yeah I didn't handle that case because it's an illegal input anyway. The sanitizing process can be tested and strengthened in the next version.


> > +      let [name, value] = param.split('=');
> > +      if (name == 'page') {
> > +        pageNo = parseInt(value);
> 
> Use `parseInt(value, 10)` If you don't want "0x123" be treated as a
> hexadecimal number. Otherwise, I'm fine with it now.
> 
There are no hexadecimal inputs generated from PDF.js. We can decide whether to allow that or just refuse that.

> > +    if (scale) {
> > +      if (this._isValidFitting(scale)) {
> > +        this._fitting = scale;
> > +      } else {
> > +        this._fitting = 'none';
> > +        this._zoom = parseInt(scale) / 100;
> 
> `_zoom` will become 0 if `scale` isn't a valid fitting nor a number. Can we
> avoid that?
> 
Sure I can try to give it a default value.
> ::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:572
> (Diff revision 1)
> > +        this._zoom = parseInt(scale) / 100;
> > +      }
> > +    }
> > +
> > +    if (Number.isInteger(pageX) && Number.isInteger(pageY)) {
> > +      let screenPos = this._getScreenCooridnate(pageNo, pageX, pageY);
> 
> According to this rule, a user will see the next page if `pageY` is omitted
> as it's treated as 0. After tring PDF.js' demo page, I think it treats the
> omitted `pageY` as the height of the current page so a user can see the
> beginning of the page specified by `page` argument. Could you confirm what
> rule PDF.js is using for this? Thanks.
> 
This version needs both pageX and pageY are present to be recognized as vaild. But yeah I can test what PDF.js do for just one coordinate.
That would be a very rare case which may come from user hand-typing since PDF.js always generates two coordinates together. I think very few users hand-type the coordinate though.
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

https://reviewboard.mozilla.org/r/114924/#review117774

Like comment 5 mentioned, I think we need to test on another machine with different resolution. As of the one more time refresh, if this causes performance issue or regress user experience, we should fix that.

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm:1550
(Diff revision 1)
>          type: "fullscreenChange",
>          fullscreen: evt.data.fullscreen
>        });
>      });
> +
> +    mm.addMessageListener("ppapi.js:hashchange", (evt) => {

nit: `this.mm`

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm:1556
(Diff revision 1)
> +      this.notifyHashChange(evt.data.url);
> +    });
> +  }
> +
> +  notifyHashChange(url) {
> +    let hash = url.split('#')[1];

could we use new URL(string) to get hash instead of parsing by ourselves?

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm:1694
(Diff revision 1)
>        case 'save':
>          this.mm.sendAsyncMessage("ppapipdf.js:save");
>          break;
> +      case 'setHash':
> +        this.mm.sendAsyncMessage("ppapi.js:setHash", message.hash);
> +        break;

I'm confused by having two prefix on async messages. @Luke, could you remind me of the difference between `ppapi.js:` and `ppapipdf.js`?

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:35
(Diff revision 1)
>      // Similar to above. Will notify runtime only if "_notifyRuntimeOfResized()"
>      // gets a different value.
>      this._runtimeSize = this.getBoundingClientRect();
>      this._runtimeOnResizedListener = [];
>  
> +    this._initViewBookmark = null;

The naming and the usage of this variable are not clear to me. It seems like a flag to indicate if we have set a hash before? Then why not to be a bool but a hash string? The name sounds like a function type though.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:541
(Diff revision 1)
> +   */
> +  _jumpToViewBookmark(hash) {
> +    if (!this._documentDimensions) {
> +      this._initViewBookmark = hash;
> +      return;
> +    }

... and why is this related to `_documentDimensions`? I feel we need a comment here.
Attachment #8840399 - Flags: feedback?(ehung)
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

https://reviewboard.mozilla.org/r/114924/#review117774

> I'm confused by having two prefix on async messages. @Luke, could you remind me of the difference between `ppapi.js:` and `ppapipdf.js`?

One is for PDF only and the other is for both PDF and Flash. So you're right! In this case we should use `ppapipdf.js`.
(In reply to KM Lee [:rexboy] from comment #9)
> (In reply to Luke Chang [:lchang] from comment #7)
> > > +      let [name, value] = param.split('=');
> > > +      if (name == 'page') {
> > > +        pageNo = parseInt(value);
> > 
> > Use `parseInt(value, 10)` If you don't want "0x123" be treated as a
> > hexadecimal number. Otherwise, I'm fine with it now.
> > 
> There are no hexadecimal inputs generated from PDF.js. We can decide whether
> to allow that or just refuse that.

I don't have strong opinions but would prefer always treating it as a decimal number to align with MDN's suggestion:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters
Some little update:

the <a> tag of bookmark tag was kept updating when each time screen position is changed in PDF.js. So when user click on the <a> link, hash is automatically updated as the page location.

https://github.com/mozilla/pdf.js/blob/573236e3adcdaebe31bb2d24d22ff6760a679f85/web/pdf_viewer.js#L812


Unfortunately this technique doesn't work in our case based on my experiment. I guess the nested structure of Iframe prevents us from doing that in the same way. 
Changing target to _top, _parent, and etc. doesn't work either.

So I think we will just to keep the current implementation of using button, click event and async message.
(In reply to Luke Chang [:lchang] from comment #11)
> One is for PDF only and the other is for both PDF and Flash. So you're
> right! In this case we should use `ppapipdf.js`.

What about the existing ppapi.js:fullscreenchange then?
I suppose you'd like to change that too?
(In reply to KM Lee [:rexboy] from comment #14)
> (In reply to Luke Chang [:lchang] from comment #11)
> > One is for PDF only and the other is for both PDF and Flash. So you're
> > right! In this case we should use `ppapipdf.js`.
> 
> What about the existing ppapi.js:fullscreenchange then?
> I suppose you'd like to change that too?

Not really.

"ppapi.js:fullscreenchange" is only used in PDF indeed. However, since it's part of fullscreen APIs such as "ppapi.js:setFullscreen", which is used in both PDF and Flash, we intended to make it align with others.
Tested with linux, mac+retina screen, mac+ordinary screen and seems they works completely the same way.
I tested for about 5 to 6 documents and haven’t found a document with different scale of coordinate yet.

So I think we realized the general cases well. Maybe we can leave those special cases (if exist) when we find one.
Attachment #8840399 - Flags: review?(ehung)
Attachment #8840399 - Flags: feedback?(lchang)
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

https://reviewboard.mozilla.org/r/114924/#review119950

The patch looks good to me except the comment I pointed out. r+.
@Luke, please take a look and see if your previous comments have been addressed. Thanks.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:510
(Diff revision 2)
> +    let currentPos = this._nextPosition || this.getScrollOffset();
> +    let pageDimension = this._pageDimensions[this._page];
> +    // Page coordinate uses Cartesian coordinate system which locates its origin
> +    // at bottom-left of a page.
> +    // The coordinate used in PDF.js is scaled by 0.75 with respect to PDFium so
> +    // we keep it here for backward compability.

per offline discussion, let move 0.75 to a const variable, and put these comments there.

nit for the comments:
`We use Cartesian coordinate system which is different from PDFium engine. Our origin is at bottom-left of every page, while PDFium counts y position continuously from the top of page 1.`
`Moreover, the coordinate used in PDF.js is scaled by 0.75 for some reason, so we keep it here for backward compability.`

I think this one should be kept inline the code as it describes the default value.
`Both pageX and pageY are omittable, and in this case, we assume the most top and left corner of the page as their default values.`

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:534
(Diff revision 2)
> +    // at bottom-left of a page.
> +    // The coordinate used in PDF.js is scaled by 0.75 with respect to PDFium so
> +    // we keep it here for backward compability.
> +
> +    // Both pageX and pageY are omittable and we assume top or left side of the
> +    // page as their default.

see above.
Attachment #8840399 - Flags: review?(ehung) → review+
I will trigger auto-land when Luke gives his f+.
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

https://reviewboard.mozilla.org/r/114924/#review119984

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:568
(Diff revision 2)
> +      params[name.toLowerCase()] = value.toLowerCase();
> +    });
> +
> +    let pageNo = parseInt(params.page, 10);
> +    pageNo = Math.max(0, Math.min(this.pageCount - 1, pageNo - 1));
> +    pageNo = Number.isNaN(pageNo) ? this._page : pageNo;

I would swap this line with the previous line since `Math.max` and `Math.min` won't introduce `NaN`.
Comment on attachment 8840399 [details]
Bug 1338095 - [Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang,

Overall looks good. Thanks.
Attachment #8840399 - Flags: feedback?(lchang) → feedback+
Patch updated but seems I typed wrong name here so the r+ is cancelled by mozreview :-/
Sorry for that. The patch still carries valid r+ from evelyn and f+ from luke.

Evelyn may you have autoland for it, thanks!
Flags: needinfo?(ehung)
Pushed by ehung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24a43c8ae122
[Mortar] Implement "View bookmark" button in PDF viewer. f=lchang, f=lochang, r=evelyn
done.
Flags: needinfo?(ehung)
Attachment #8840399 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/24a43c8ae122
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
[Bugday-20170315]
The bug is verified.
OS: Windows 7
Browser:Nightly 55
(In reply to krithivelan25 from comment #27)
> [Bugday-20170315]
> The bug is verified.
> OS: Windows 7
> Browser:Nightly 55

Hi, thank you for watching this bug, but you probably are not verifying this implementation because we haven't enable it on Nightly. 

The PDF viewer UI you see on Nightly is our pdf.js solution, which should be fine without any problem. This bug is for another new PDF viewer solution, which is still under construction. Thanks!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: