[4.0] Custom Elements everywhere: iframe wrapper #18916
Conversation
|
Please add a title attribute All or <iframe> elements in the document must have a title that is not empty to describe their contents to screen reader users. The title attribute is not interchangeable with the name attribute. The title labels the frame for users; the name labels it for scripting and window targeting. The name is not presented to the user, only the title is. |
|
Sometimes wish iframes would just become obselete |
|
and when you look at where we use them in the admin ui you will see how abused they are and how they are not used correctly. |
|
@brianteeman shall we do the a11y part in another PR? |
|
i dont see why. this is a total replacement of the current iframe code so why leave it til after this one has been merged. its just duplicating work for you and for testers. |
| @@ -98,11 +98,11 @@ public function display($tpl = null) | |||
| // Auto height control | |||
| if ($params->def('height_auto')) | |||
There was a problem hiding this comment.
Can be changed to:
$wrapper->load = false;
if ($params->def('height_auto'))
{
$wrapper->load = true;
}
There was a problem hiding this comment.
If you're really trying to simplify this, then just do $wrapper->load = $params->def('height_auto'); since it's already returning the boolean value you want to assign.
|
Ok, so title will be the menu title for the component and the module title for the modules? |
| adjustHeight() { | ||
| let height = 0; | ||
| const doc = this.iframe.contentWindow.document; | ||
| height = doc.body.scrollHeight; |
There was a problem hiding this comment.
may aswell make this let height = doc.body.scrollHeight; and remove let height = 0; above
There was a problem hiding this comment.
it's just a fallback, can be written also as
const height = doc.body.scrollHeight || 0;
|
For _wrapper then yes that will should be ok but note it won't be in other places in the core that we have iframe it won't be |
|
@dgt41 can this PR be tested, can you give Test Instructions? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18916. |
ghost
mentioned this pull request
|
Reminder for @dgt41 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18916. |
|
|
thanks for Info, @C-Lodder (i added them in first Comment so Tester don't have to search in Thread). @dgt41 can you please resolve conflicting Files so this Pull Request can be tested? |
|
@franz-wohlkoenig done |
|
I have tested this item ✅ successfully on fcb898e
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18916. |
|
@dgt41 can you resolve these conflicting files. |
|
@Anu1601CS done |
|
@Anu1601CS does that happen when you save a module? |
|
@laoneo yes |
|
Can you test if my pr is working? |
|
I have got problems with this brach. Error Message |
|
@C-Lodder Why do people need to complicate things, and don't want to make it simpler? |
|
@BaleshSrle can you explain me what is complicate here? I'm just using the W3C standards Custom Elements, is that complicated? If so you should rethink your involvement Anyways I've asked people with merge right to first merge your PR and then I'll patch things here |
|
@BaleshSrle - I have no idea what you're on about and why you're asking me |
|
@C-Lodder I don't know, maybe because I can see widsom on you. |
|
Can you fix the conflicts on this one please? |
|
@laoneo done |
|
Preview component + module (inception mode): |
|
Can you remove the commented out code please |
Pull Request for Issue # .
Summary of Changes
Changes for the com_wrapper and mod_wrapper to use custom elements.
This fixes:
Testing Instructions (by @C-Lodder)
Expected result
Actual result
Documentation Changes Required
None. The new code reflects the old behaviour!