Caret monocle doesn't position properly after ContentAreaObserver adjusts content dims

RESOLVED INVALID

Status

RESOLVED INVALID
6 years ago
5 years ago

People

(Reporter: jimm, Unassigned)

Tracking

Trunk
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

6 years ago
STR:

1) tap a form input to bring up the caret and the keyboard

result: when the soft keyboard comes up the caret is not adjusted to reflect the new position of the form.

also, in some cases the form input will be pushed off screen, so we have to be careful about when we display monocles.
(Reporter)

Updated

6 years ago
Summary: Caret monocle doesn't position properly after formassistant adjusts content dims → Caret monocle doesn't position properly after ContentAreaObserver adjusts content dims
(Reporter)

Comment 1

6 years ago
Created attachment 728757 [details] [diff] [review]
patch

details:

1) move ContentAreaObserver into its own file
2) fire an new event ('ViewableSizeChanging') right before we change the viewable area
3) hide monocles on 'ViewableSizeChanging'
4) trigger a recalculation of the monocle position after 'ViewableSizeChanged' fires, which also shows the monocles again.
5) track the current selection mode in SelectionHandler so we can reuse "Browser:SelectionUpdate" for this.
Assignee: nobody → jmathies
Attachment #728757 - Flags: review?(netzen)
(Reporter)

Updated

6 years ago
Attachment #728757 - Attachment is patch: true
(Reporter)

Updated

6 years ago
OS: Windows 7 → Windows 8 Metro
Comment on attachment 728757 [details] [diff] [review]
patch

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

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +856,5 @@
>  
> +  // triggered when the viable size changes due to keyboard display
> +  _onViewableSizeChanging: function _onViewableSizeChanging() {
> +    if (this._startMark) {
> +      this.startMark.visible = false;

I don't think visible has a setter. I think the proper way to do this is:
this.startMark.hide();

@@ +859,5 @@
> +    if (this._startMark) {
> +      this.startMark.visible = false;
> +    }
> +    if (this._endMark) {
> +      this.endMark.visible = false;

I think this should be this.endMark.hide();

@@ +862,5 @@
> +    if (this._endMark) {
> +      this.endMark.visible = false;
> +    }
> +    if (this._caretMark) {
> +      this.caretMark.visible = false;

I think this should be this.caretMark.hide();
Attachment #728757 - Flags: review?(netzen) → review+
(Reporter)

Comment 3

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #2)
> 
> I don't think visible has a setter. I think the proper way to do this is:
> this.startMark.hide();

doh, nice catch.
(Reporter)

Comment 4

6 years ago
I'm going to delay this until we have a chance to look at the content area changes we make when he keyboard is displayed.

There are a couple problems here - 

1) content doesn't always reposition for some reason. I can reproduce repositioning on some sites but not others.
2) ViewableSizeChanged doesn't always coincide with reflow / paint.
Blocks: 831982
No longer blocks: 851388
(Reporter)

Updated

6 years ago
No longer blocks: 831982
(Reporter)

Updated

6 years ago
Blocks: 850413
(Reporter)

Updated

6 years ago
Depends on: 855237
(Reporter)

Updated

6 years ago
Assignee: jmathies → nobody
(Reporter)

Updated

6 years ago
Attachment #728757 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Reporter)

Updated

6 years ago
No longer depends on: 855237
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.