Closed Bug 854182 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: jimm, Unassigned)

References

Details

Attachments

(1 obsolete file)

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.
Summary: Caret monocle doesn't position properly after formassistant adjusts content dims → Caret monocle doesn't position properly after ContentAreaObserver adjusts content dims
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #728757 - Attachment is patch: true
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+
(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.
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: caret-sel
No longer blocks: 831982
Blocks: skb
Depends on: 855237
Assignee: jmathies → nobody
Attachment #728757 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
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.