ElementDocument::FocusFirstTabElement

[viciious]

bool FocusFirstTabElement(void) { return FocusNextTabElement(this, true); }

We use this function to focus on the first "tabbable" element when the document is displayed for the first time, if possible. Make sense for modal dialogs, input forms and such.

Please consider adding this function to the public API or maybe, as an alternative, a new Focus flag with similar functionality.

[mikke89]

It seems good, but what do you think about an autofocus attribute so it's easier to customize the element to focus?

[viciious]

Yeah, I like the autofocus idea. The code should be able to handle multiple autofocus elements too and always pick the first one.

Another thing to note is that the focus should not change when the document is hidden away and shown again.

The property alone isn't going to allow you to reset the focus though.

[mikke89]

Alright, that should be doable. So only set it during document load for example?

[viciious]

I think it should be postponed until before the first Show() call with the focus flag set, since autofocus is dependent on visibility state of the elements and a whole lot can happen between load and show events...

[mikke89]

Hm, maybe a FocusElement flag during document show to control it?

[viciious]

That was one of the alternative suggestions in my original post :)

[mikke89]

Right :P

[mikke89]

Let me know how it works for you. :)

[viciious]

What I don't like about this approach is that doing
doc->Show( Rml::Core::FocusFlag::Focus );

or

doc->Show( Rml::Core::FocusFlag::FocusDocument );

on an already displayed document resets the modal flag so I need to keep track of it myself, which is quite inconvenient.

void Document::Focus() {
	auto *doc = rocketDocument;
	if( doc == nullptr ) {
		return;
	}
	doc->Show( doc->IsModal() ? Rml::Core::FocusFlag::ModalDocument : Rml::Core::FocusFlag::FocusDocument );
}

void Document::FocusFirstTabElement() {
	auto *doc = rocketDocument;
	if( doc == nullptr ) {
		return;
	}
	doc->Show( doc->IsModal() ? Rml::Core::FocusFlag::Modal : Rml::Core::FocusFlag::Focus );
}

[mikke89]

This is how it worked before, right? What behavior do you propose?

[mikke89]

Maybe like this?

[viciious]

This is how it worked before, right? What behavior do you propose?

Well, the original proposal was to add a new method called FocusOnFirstTabElement(), which would only do what the name suggests :)
I don't think libRocket's Focus() messed up the modal flag either, but I could be wrong here.

Maybe like this?

Is it really necessary to change the modal flag on visibility status change?

On a related note, in ComputedValues.h:

Visibility visibility = Visibility::Visible;

Wouldn't it be better/make more sense to default this to Visibility::Hidden? Also, in ElementDocument::ProcessHeader:

SetProperty(PropertyId::Visibility, Property(Style::Visibility::Hidden));

doesn't set the linked computed value to corresponding value. I know this is expected behavior but it does cause certain problems with iframes: querying the parent document's visibility status on initialization always returns true.

[mikke89]

Well, the original proposal was to add a new method called FocusOnFirstTabElement(), which would only do what the name suggests :)
I don't think libRocket's Focus() messed up the modal flag either, but I could be wrong here.

Okay, how about a separate FocusDocument(FocusFlag) function which does not modify visibility or modal status? Leave Show() like it was previously.

The Focus() function is an element function and would only focus the document element itself. We could override this instead, but I think it would be a bit weird if Element::Focus has different behavior than ElementDocument::Focus?

Is it really necessary to change the modal flag on visibility status change?

I think it would be very unexpected in the case of calling Show(Focus) on a hidden document suddenly has modal status.

On a related note, in ComputedValues.h:

Visibility visibility = Visibility::Visible;

Wouldn't it be better/make more sense to default this to Visibility::Hidden?

Oh no this would be very strange if all elements started out hidden. The default values must correspond to the default property value.

Also, in ElementDocument::ProcessHeader:

SetProperty(PropertyId::Visibility, Property(Style::Visibility::Hidden));

doesn't set the linked computed value to corresponding value. I know this is expected behavior but it does cause certain problems with iframes: querying the parent document's visibility status on initialization always returns true.

Yeah, it could make sense to run UpdateProperties after loading the header, which should solve this issue.

[mikke89]

Maybe like this:

/**
	 ModalFlag used for controlling the modal state of the document.
		None: Remove modal state.
		Modal: Set modal state, other documents cannot receive focus.
		Previous: Modal state unchanged.

	FocusFlag used for displaying the document.
	   None: No focus.
	   Auto: Focus the first tab element with the 'autofocus' attribute or else the document.
	   Previous: Focus the previously focused element in the document.
	   Document: Focus the document.
*/
enum class ModalFlag { None, Modal, Previous };
enum class FocusFlag { None, Auto, Previous, Document };

ElementDocument::Show(ModalFlag modal_flag = ModalFlag::None, FocusFlag focus_flag = FocusFlag::Auto);

[viciious]

I believe Current would be a more fitting term here than Previous

[mikke89]

Hm, I don't really like Previous either, but Current is also very strange, especially when showing the document.

[viciious]

Keep or NoChange then maybe? :P

[mikke89]

Alright, Keep it is: 07805b7 :)

[mikke89]

Are you okay with the solution, can we close this?

[viciious]

Yup, I'm fine with it, thanks!